-
Notifications
You must be signed in to change notification settings - Fork 378
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
feat: add base flag to info command #3779
base: main
Are you sure you want to change the base?
Conversation
auto& base = config.insert( | ||
Configurable("base", false).group("cli").description("Display base environment path.") | ||
); | ||
subcom->add_flag("--base", base.get_cli_config<bool>(), base.description()); |
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.
Can mamba info --base
be tested?
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 have no idea how to test it. Open to suggestions!
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.
What do you think of testing that the usual information is not present and that an existing path is output?
micromamba/src/info.cpp
Outdated
using namespace mamba; | ||
|
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.
What is this used for?
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.
It's for Configurable
, which is a mamba::Configurable
.
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.
OK, in this case I propose https://github.com/mamba-org/mamba/pull/3779/files#r1936931327.
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.
Indeed, using namespace
directive should be avoided as much as possible.
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.
It is done in /mamba/micromamba/src/list.cpp , which is why I originally went for it in /mamba/micromamba/src/info.cpp (but it used only once here, and more in list.cpp).
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.
OK, we can remove (broad) using namespace
directives in another PR.
micromamba/src/info.cpp
Outdated
@@ -22,6 +25,11 @@ set_info_command(CLI::App* subcom, mamba::Configuration& config) | |||
init_info_parser(subcom, config); | |||
static bool print_licenses; | |||
|
|||
auto& base = config.insert( | |||
Configurable("base", false).group("cli").description("Display base environment path.") |
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.
How about using this if this is possible?
Configurable("base", false).group("cli").description("Display base environment path.") | |
mamba::Configurable("base", false).group("cli").description("Display base environment path.") |
Co-authored-by: Klaim <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]> Co-authored-by: Johan Mabille <[email protected]> Co-authored-by: Hind Montassif <[email protected]>
…mamba-org#3792) Signed-off-by: Julien Jerphanion <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>
…rg#3788) Signed-off-by: Julien Jerphanion <[email protected]>
Adds one of the missing sub-command #3535