-
-
Notifications
You must be signed in to change notification settings - Fork 611
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 Environment Override #1842
base: main
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.
I am not a maintainer, but would love to see this change to be merged.
) | ||
|
||
assert out.exit_code == 0, out | ||
assert expected_output == out.stdout |
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 would love to see an extra test, where we check that we can override all of the env dict values to ensure that we support passing multiple values to the --override-environment
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 added a test with multiple environment markers passed, as well as a test that uses all valid markers one at a time.
@click.option( | ||
"--override-environment", | ||
multiple=True, | ||
type=(str, str), |
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 we add validation on the values that people can override? I would expect this to fail if I pass --override-environment foo bar
to the CLI. If this can be shown in the help
to tell the user what environment markers can be overridden, it would improve the experience as well.
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.
Added argument validation
specify the environment when gathering dependencies, allowing for cross-environment | ||
fetching. However, a different ``requirements.txt`` must still be generated per | ||
environment. It is recommended to override all keys in `PEP 508 environment markers`_ | ||
when targetting a different environment so the environment is fully defined. |
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 would be great to have a short example in the README
.
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.
Added an example for a 'typical' Linux machine
Allow the pip environment used for evaluating environment markers to be overriden, so requirements can be compiled for an environment different than the user's current environment.
for more information, see https://pre-commit.ci
@aignas Thanks for the comments! I made some updates based on your feedback, let me know if there are any remaining concerns you have. @atugushev I saw you are one of the leads on this project, do you think this feature makes sense? |
Allow the pip environment used for evaluating environment markers to be overriden, so requirements can be compiled for an environment different than the user's current environment.
Contributor checklist
Maintainer checklist
backwards incompatible
,feature
,enhancement
,deprecation
,bug
,dependency
,docs
orskip-changelog
as they determine changelog listing.