Skip to content

Conversation

@Kr4is
Copy link

@Kr4is Kr4is commented Mar 22, 2021

This hook was failing due to a ModuleNotFoundError: No module named 'yaml'. I have updated the setup config to add pyyaml>=5.1, fixing that error.

Before:

imagen

After:

imagen

@cheshirekow
Copy link
Owner

yaml isn't a requirement unless you want to use yaml config (python or json config formats require no additional modules).

@Kr4is
Copy link
Author

Kr4is commented Jul 3, 2021

First of all, sorry for the late reply, it has been a very busy month.

I see your point, but in my opinion the precommit hook should support the whole cmake format utility. That means support all formats witch the original tool supports, independent if you use python, json or yaml at your config. Thats also allow all people to use the extension only adding the hook without change theoir current configuration, if you use yaml for the configuration in your project, you can still using it, without need doin modifications.

I think that the pyyaml module is not a big overload, from my point of view is better having the full real utility instead of do not install a small module.

Finally, thank you for creating and maintaining this hook! I am currently using it in several projects and it helps me to keep these files in order and in a common format!

@m-kuhn
Copy link

m-kuhn commented Oct 13, 2021

For reference, I am currently using the repo of @Kr4is to be able to configure via yaml file

- repo: https://github.com/Kr4is/cmake-format-precommit
  rev: v0.6.14

@Kr4is
Copy link
Author

Kr4is commented Oct 14, 2021

@m-kuhn Thats great!

Also i need to say that you can use the following in order to use the @cheshirekow repo with yaml configuration files:

-   repo: https://github.com/cheshirekow/cmake-format-precommit
    rev: v0.6.13
    hooks:
    -    id: cmake-format
         additional_dependencies: [pyyaml>=5.1]
    -    id: cmake-lint
         additional_dependencies: [pyyaml>=5.1]

greetings!

@m-kuhn
Copy link

m-kuhn commented Oct 14, 2021

Thanks for pointing that out.
In this case I'll switch back with this trick 🙏

Would be nice to apply your proposed fix or have the precommit hook print this hint instead of the import error.

@Andreagit97
Copy link

Hi, @Kr4is thank you for this PR, I agree with @m-kuhn that it would be nice to see it merged or have the pre-commit hook print this hint instead of the import error. Just a marginal question, do I have to provide a particular option to the cmake-format pre-commit hook to specify the path of my yaml configuration file or it is able to detect it by itself?

@Kr4is
Copy link
Author

Kr4is commented Jun 15, 2022

Hello @Andreagit97 ,

As can be seen in the documentation of cmakelang we have two options.
The first one is to specify with the -c parameter where is the configuration we want to use. The second is to use the predefined name to find it, in the case of configurations in yaml format the name should be the following: .cmake-format.yaml, being this file in the root of the repository.

I hope my answer has been helpful!

Best regards and thanks for the comment!

@Andreagit97
Copy link

Hello @Andreagit97 ,

As can be seen in the documentation of cmakelang we have two options. The first one is to specify with the -c parameter where is the configuration we want to use. The second is to use the predefined name to find it, in the case of configurations in yaml format the name should be the following: .cmake-format.yaml, being this file in the root of the repository.

I hope my answer has been helpful!

Yes, thank you very much!

Best regards and thanks for the comment!

@brccabral
Copy link

@m-kuhn Thats great!

Also i need to say that you can use the following in order to use the @cheshirekow repo with yaml configuration files:

-   repo: https://github.com/cheshirekow/cmake-format-precommit
    rev: v0.6.13
    hooks:
    -    id: cmake-format
         additional_dependencies: [pyyaml>=5.1]
    -    id: cmake-lint
         additional_dependencies: [pyyaml>=5.1]

greetings!

thanks @Kr4is

Copy link

@gegles gegles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@gegles
Copy link

gegles commented Feb 20, 2024

Could this be merged finally? Thx!

@gegles
Copy link

gegles commented Mar 8, 2024

Hello, why can this not be merged? What's the hold out?

MarkoSagadin added a commit to IRNAS/irnas-zephyr-template that referenced this pull request Apr 30, 2024
tbeu added a commit to tbeu/matio that referenced this pull request Oct 19, 2024
@3nids
Copy link

3nids commented Dec 14, 2024

This is still an issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants