Fix display of env activation message and co #3715
Conversation
| // Print activation message only if the environment is freshly created | ||
| if (create_env) | ||
| { | ||
| print_activation_message(ctx); | ||
| } | ||
|
|
There was a problem hiding this comment.
Currently the message is shown for installations and updates if the target prefix is different than the currently activated one. Should we preserve this behavior?
Also, should the message rather be printer after the call to install_for_other_pkgmgr (namely just before the else)?
There was a problem hiding this comment.
Currently the message is shown for installations and updates if the target prefix is different than the currently activated one. Should we preserve this behavior?
Indeed, and I was going to keep it that way but I realized conda was only showing activation message with environment creation, and that actually makes more sense and is less verbose (recommending env activation sounds relevant when the user creates a new environment but seems redundant when installing/updating).
For now, I suggest to do it this way, and if we want to restore the previous behavior, that should be straightforward by adding print_activation_message where needed.
Also, should the message rather be printer after the call to
install_for_other_pkgmgr(namely just before theelse)?
Good question! I put it right after the transaction execution to preserve the order of printing messages (since it was inside Transaction::execute()). But that could be discussed: either consider that the transaction is finished and print the activation message, and then handle pip packages (or anything else?) considering we do not recommend it (cf. warning that we added recently) or print the activation message after everything is done including things we don't really approve.
Note however that install_for_other_pkgmgr does not involve Transaction so it's supposed to be done with before handling other pkg managers.
In my opinion, let's keep it as it is.
| // Print activation message only if the environment is freshly created | ||
| if (create_env) | ||
| { | ||
| print_activation_message(ctx); | ||
| } | ||
|
|
There was a problem hiding this comment.
Same suggestions as above.
Side-note: are there a way we could factor (parts of) install_explicit_with_transaction and install_specs_impl in another PR?
There was a problem hiding this comment.
Side-note: are there a way we could factor (parts of)
install_explicit_with_transactionandinstall_specs_implin another PR?
Depends on what we want to refactor and if we need some parts in the codebase reworked first. But we can look into it.
mamba env remove#3712conda).