Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specifying extra arguments to CMake format #37

Conversation

krypty
Copy link
Contributor

@krypty krypty commented Sep 29, 2023

Hello,

The configuration format is described here: https://github.com/cheshirekow/cmake_format#configuration

Please tell if I got any wrong with regards to contributing to this project.

Copy link
Owner

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I'm a bit unsure about this one, as cmake-format already provides an implicit way to add config files using the file path, which for example is it convenient to specify different configs for different directories:

Otherwise, cmake-format will search the ancestry of each infilepath looking for a configuration file to use. If no configuration file is found it will use sensible defaults.
A automatically detected configuration files may have any name that matches .?cmake-format(.yaml|.json|.py).

But I do see how it can be useful extra arguments to the cmake-format command. How instead we add a more generic option CMAKE_FORMAT_EXTRA_ARGS to allow users to provide arbitrary extra arguments to the CMake format executable?

@krypty
Copy link
Contributor Author

krypty commented Oct 9, 2023

Sure good idea, I will come back with the more generic proposition you made i.e. CMAKE_FORMAT_EXTRA_ARGS.

@krypty krypty force-pushed the add-option-to-specify-cmake-format-config branch from a7cb20b to d495d27 Compare October 11, 2023 21:45
@krypty krypty force-pushed the add-option-to-specify-cmake-format-config branch from d495d27 to 9f77512 Compare October 11, 2023 21:48
@krypty
Copy link
Contributor Author

krypty commented Oct 11, 2023

OK, I think I got it right. Tell me if that's not the case :) Should we change the PR title?

@TheLartians TheLartians changed the title Add option to specify a custom configuration file for cmake-format Allow specifying extra arguments to CMake format Nov 1, 2023
Copy link
Owner

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the changes! I've gone ahead and updated the PR title. ;-)

@TheLartians TheLartians merged commit db37c4e into TheLartians:master Nov 1, 2023
3 checks passed
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.

2 participants