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

Suggest a way of cleaning dist if non-empty #7665

Merged
merged 1 commit into from
Feb 2, 2020

Conversation

webknjaz
Copy link
Member

This is extracted from #7654.

@webknjaz webknjaz requested a review from pradyunsg January 28, 2020 15:05
@chrahunt chrahunt added the skip news Does not need a NEWS file entry (eg: trivial changes) label Jan 29, 2020
@chrahunt chrahunt force-pushed the misc/nox-build-cleanup-suggestion branch from c60f588 to fb2598a Compare January 31, 2020 01:54
@chrahunt
Copy link
Member

Rebased on master to fix CI.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

I have no issues with this merging if someone else in @pypa/pip-helpers or @pypa/pip-committers think this is useful, but I don't think I'd be merging this in myself.

As worded, the new suggestion seems to nudge toward deleting dist/ without taking a look. It's easy enough to automate the deleting of an unnecessary directory but I'd figured that we should not be deleting dist/ by default here. That's because I prefer automation that avoids destructive actions in situations where it's not known what the cause for the fault is or what the correct corrective action should be (do they care about what's in dist/? should they? how can we know? can the action be undone?).

OTOH, I'm sure that someone making a pip release would know what to do to delete a folder, so providing a suggestion isn't required. Plus, some part of me looks at the git command here and it is not clear what it does or why this is the correct suggestion. FWIW, it is the only cross platform suggestion that I could think of too.

@pradyunsg pradyunsg added C: automation Automated checks, CI etc type: maintenance Related to Development and Maintenance Processes labels Jan 31, 2020
@webknjaz
Copy link
Member Author

@pradyunsg that's why I added -i there to make it interactive. So Git will basically ask to confirm deleting things.

@webknjaz
Copy link
Member Author

Also that's why it says "you can", not "you must"

@pradyunsg
Copy link
Member

Ah well. The interactive bit does sound useful -- I just tried it locally and, yea, makes sense. Me likey this now.

@chrahunt chrahunt merged commit a635e0d into pypa:master Feb 2, 2020
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Mar 10, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: automation Automated checks, CI etc skip news Does not need a NEWS file entry (eg: trivial changes) type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants