Skip to content
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

Allow uv pip sync to clear an environment with opt-in #4517

Merged
merged 5 commits into from
Jul 2, 2024

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Jun 25, 2024

Closes #4516

Open to some deliberation about the opt-in strategy here.

@zanieb zanieb added the cli Related to the command line interface label Jun 25, 2024
@zanieb zanieb requested a review from charliermarsh June 25, 2024 16:18

/// Allow sync of empty requirements, which will clear the environment of all packages.
#[arg(long, overrides_with("no_allow_empty_requirements"))]
pub allow_empty_requirements: bool,
Copy link
Member

@charliermarsh charliermarsh Jun 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer --allow-empty, I don't think it's ambiguous.

I think this also needs to be added to the PipOptions and respected in persistent configuration, and it should probably be accepted on pip install too? Though not sure what it would mean in that case so maybe not.

Copy link
Member Author

@zanieb zanieb Jun 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If pip install exits with success when the requirements are empty I don't think it would mean anything there.

I can shorten it, though I find it a bit ambiguous.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it makes sense to include in the persistent configuration too? It only seems relevant for this command though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it should be in the persistent configuration -- that's the contract for the pip table (e.g., it includes things that only apply to pip-compile).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find --allow-empty-requirements to be more verbose than necessary but not a strong opinion, it's fine.

@charliermarsh
Copy link
Member

@zanieb - I'll add to config and merge.

@zanieb
Copy link
Member Author

zanieb commented Jul 2, 2024

@charliermarsh thanks!

@zanieb zanieb closed this Jul 2, 2024
@zanieb zanieb reopened this Jul 2, 2024
@zanieb
Copy link
Member Author

zanieb commented Jul 2, 2024

Sorry got too excited

@charliermarsh charliermarsh enabled auto-merge (squash) July 2, 2024 12:26
@charliermarsh charliermarsh force-pushed the zb/sync-allow-empty branch from 15e9dc7 to a361c69 Compare July 2, 2024 12:40
@charliermarsh charliermarsh merged commit 2c0cb6e into main Jul 2, 2024
47 checks passed
@charliermarsh charliermarsh deleted the zb/sync-allow-empty branch July 2, 2024 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command line interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uv pip sync fails to clear environment when given an empty requirements list
2 participants