-
Notifications
You must be signed in to change notification settings - Fork 187
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
feature: allow additional arguments after shorthand syntax #546
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for the PR! In general I like the idea of mixing the short syntax with additional arguments and think this is a good approach in doing so. Could you perhaps add an integration test case for this?
Also I'm not sure but it could be that the cmake-format config might need to be updated for this new syntax.
Yes, there are a few things that need to be updated. I wanted to discuss, if there is any chance on success before investing to much time. |
4eaa26b
to
1dfebcd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the changes look good, just want to make sure that the test actually does what it should. One thing that I notice from the readme changes is that having a mix of single + named arguments looks quite inconsistent and therefore harder to read.
How would you feel if we instead make a compromise by introducing a URI
argument that takes the role of the single argument instead?
This would allow us to do
CPMAddPackage(
URI "gh:nlohmann/json@3.9.1"
OPTIONS "JSON_BuildTests OFF"
)
Instead of
CPMAddPackage("gh:nlohmann/json@3.9.1"
OPTIONS "JSON_BuildTests OFF"
)
To me this looks much nicer, even though I know CMake itself likes to use this mix of syntax. Of course, using only the single argument syntax should continue to be valid.
|
||
create_with_commit_sha.() | ||
update_to_version_1.() | ||
update_with_option_off_and_build.() | ||
update_with_option_off_and_build_with_shorthand_syntax.() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like having this run as part of a larger test makes reasoning about the test itself difficult. Do you think you could move checking the shorthand extra arguments into its own isolated test case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure 👍
I am not so sure of the
Con:
I like to think about it similar to how |
This allows to combine the shorthand syntax with additional arguments: ``` CPMAddPackage("gh:nlohmann/json@3.9.1" OPTIONS "JSON_BUildTests OFF") ``` This is much shorter than the longer syntax way of writing: ``` CPMAddPackage( NAME nlohmann_json VERSION 3.9.1 GITHUB_REPOSITORY nlohmann/json OPTIONS "JSON_BuildTests OFF" ) ```
26478df
to
668fb00
Compare
Gentleman, how is this coming along? |
The question of The only big stopper for the |
This allows to combine the shorthand syntax with additional arguments:
This is much shorter than the longer syntax way of writing: