feat: add canonical flag to list command #3777
Conversation
jjerphan
added
the
release::enhancements
libmamba/src/api/list.cpp
Outdated
| @@ -27,11 +27,12 @@ namespace mamba | |||
| bool reverse; | |||
| bool explicit_; | |||
| bool md5; | |||
| bool canonical; | |||
There was a problem hiding this comment.
Could you add a default value ( = false;) to every member of this struct? I'm not convinced that it's always initialized correctly at usage point. If something prevents you to do so, nevermind.
libmamba/src/api/list.cpp
Outdated
| std::cout << p.name << "-" << p.version << "-" << p.build_string << std::endl; | ||
| } | ||
| } | ||
| else if (options.explicit_) |
There was a problem hiding this comment.
What should happen if we ask for --explicit and --canonical?
Here, it would consider it as --canonical only (because it's the first condition), but if we want it lead to an additional behavior and print both lines (build_string and url) we should use an if instead of else if.
Same for remaining cases (should the else be exclusive etc).
There was a problem hiding this comment.
I had this question in mind for --canonical and --export (the next one I started working on), but missed it for --canonical and --explicit. Do we want to have the same behavior as conda? If so I would have to check what appends in these cases. If not, we would have to decide on the format of the output.
There was a problem hiding this comment.
Based on the descriptions, I think this should be exclusive indeed (I think conda does the same but gives precedence to --explicit). The question is, should we error out when multiple exclusive flags are given by the user or just ignore that and have some kind of priority that we decide on (for now it's --canonical)?
There was a problem hiding this comment.
I tested with conda. I will reproduce its behavior (explicit > canonical > export) and print a warning when two options are used.
|
A more general question, are these recently added flags supposed to be supported only with a regular output (not |
|
When the --json flag is used, we return a json with everything, no matter which other flag appears (discussed with @JohanMabille). If I add md5, all the information needed to create the outputs of the other flags would be in the json. |
So these specific flags are ignored with |
|
Yes, you're right. |
| .group("cli") | ||
| .description("Output canonical names of packages only. Ignored if --explicit.") | ||
| ); | ||
| subcom->add_flag("-c,--canonical", canonical.get_cli_config<bool>(), canonical.description()); |
There was a problem hiding this comment.
Actually I just remembered that we can use excludes option in CLI11 to have the behavior we want:
| subcom->add_flag("-c,--canonical", canonical.get_cli_config<bool>(), canonical.description()); | |
| subcom->add_flag("-c,--canonical", canonical.get_cli_config<bool>(), canonical.description())->excludes(explicit_flag); |
explicit_flag being something like:
auto* explicit_flag = subcom->add_flag("--explicit", explicit_.get_cli_config<bool>(), explicit_.description());
This way, no need to add Ignored if --explicit. in the description.
There was a problem hiding this comment.
The idea was to let the user know the behavior of the commands, so not sure about removing the message.
Should I use excludes but leave Ignored if --explicit. in the description ?
There was a problem hiding this comment.
I think excludes automatically adds a note after the flag description in the --help text (saying that it's excluding some flag/option). But yes you can try it out and do whatever you think is user friendly.
There was a problem hiding this comment.
I tried it and I don´t think that it is the behavior we want as the command doesn´t run when using excludes. I will keep the initial version.
libmamba/src/api/list.cpp
Outdated
| std::cout << "Warning: Option --canonical ignored because of --explicit \n" | ||
| << std::endl; |
There was a problem hiding this comment.
In these cases, using LOG_log_level is recommended:
| std::cout << "Warning: Option --canonical ignored because of --explicit \n" | |
| << std::endl; | |
| LOG_WARNING << "Option --canonical ignored because of --explicit"; |
But that's just a remark for the future, if we use excludes option, this won't be necessary anymore.
| assert all(i in output for i in items) | ||
| assert " " not in output | ||
|
|
||
|
|
There was a problem hiding this comment.
We can maybe combine with explicit tests (if it doesn't complicate things too much) and test the case where it should warn/abort (with excludes option)?
Adds one of the missing sub-command #3535