-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Do not require name or prefix if we are running create
with dry-run
#13941
Do not require name or prefix if we are running create
with dry-run
#13941
Conversation
CodSpeed Performance ReportMerging #13941 will not alter performanceComparing Summary
|
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.
Just a couple requests for changes. Please let me know if any seem unreasonable. Will be happy to review once more.
conda/cli/main_create.py
Outdated
from ..gateways.disk.delete import rm_rf | ||
from ..gateways.disk.test import is_conda_environment | ||
from .common import confirm_yn | ||
from .install import install | ||
|
||
if not args.name and not args.prefix: | ||
if context.dry_run: | ||
context._argparse_args.prefix = os.path.join(mktemp(), "dry-run-create-env") |
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.
context._argparse_args.prefix = os.path.join(mktemp(), "dry-run-create-env") | |
args.prefix = os.path.join(mktemp(), "dry-run-create-env") | |
context.__init__(argparse_args=args) |
I think this is slightly better. No need to access a protected property when we can simply re-initialize the object. Performance penalty should be negligible.
Also, Could we make "dry-run-create-env"
a constant and define it in conda.base.constants
. Not necessary, but would help keep the code a little more organized and provide a central place to look up this value in case it is ever needed.
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.
Are we sure we are not overriding some stuff from os.environ
with that call? 🤔 I always get nervous when I reinitialize the context outside of the tests 😬
+1 to the 2nd suggestion.
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.
Applied your suggestions. The new tests seem to be passing locally.
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.
No reason to worry about overriding stuff from os.environ
. I double checked the code. We just need to make sure we initialize it like we do in conda.cli.main.main_subshell
by pass the argparse args:
Line 72 in a261ade
context.__init__(argparse_args=pre_args)
Co-authored-by: Travis Hathaway <travis.j.hathaway@gmail.com>
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.
🚀
Looking good. Thanks for being receptive to my 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.
Thanks for doing this work, @jaimergp !
Description
Folks often rely on
create --dry-run
to run solver diagnostics. For this operation, prefixes or env names are not needed, yet they are still required in the CLI. It's common to see invocations such as:Or the shorthand (with
x
being the env name):However it should be perfectly fine to run:
This is what we see in this PR.
Checklist - did you ...
news
directory (using the template) for the next release's release notes?