-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Simplify InstallRequirement.uninstall #7422
Simplify InstallRequirement.uninstall #7422
Conversation
This is not passed by either `UninstallCommand` or `install_given_reqs` (the only two callers), so we can remove it.
`InstallRequirement.check_if_exists` is called in several places for dependency resolution and uninstall. We make a copy of the existing function and only change the name so it's possible to simplify it while only considering the uninstall use case.
Since the condition is now obviously False we can drop this code.
There are two callers to `uninstall`: 1. `pip uninstall` - as indicated, only named requirements are ever used 2. `pip install` via `req.install_given_reqs` - which since uninstall is only called when `conflicts_with` is set, must already be named In case we're worried about this being relevant for the `pip uninstall .` case (as requested in the issue tracker), we can worry about it later - all this code will look very different by the time we get around to implementing that.
In the next step we can inline this to simplify `check_if_exists_uninstall`.
We map `False` to `None` and `True` to `self.satisfied_by or self.conflicts_with`. This simplifies the uninstall function and will let us do some cancellation in check_with_exists_uninstall without too much hassle.
This is the default value which is only ever set in InstallRequirement.check_if_exists, which isn't called before this point in `pip uninstall`.
`satisfied_by` is only set to a non-`None` value by `check_if_exists`. In the error case where `satisfied_by` is not set, `check_if_exists` may set `conflicts_with`. The only other time `conflicts_with` is set is if we have taken its value from `satisfied_by` and then immediately set `satisfied_by = None`. So `satisfied_by` must be `None` here.
Since all (both) callers have this assertion, we can assert the same in the called functions.
At least not after uninstall, based on inspection of `UninstallCommand` and `InstallCommand`/`install_given_reqs`.
Since we previously enforced that `satisfied_by` and `conflicts_with` aren't used after install, we can simplify without worrying about preserving the member assignment side-effect.
pkg_resources.get_distribution either returns a Distribution or raises an exception, so this will always be true.
To make it easier to review how we simplify the rest of this function.
Since `self.satisfied_by` will always be set, as mentioned before, `self.conflicts_with` will never be returned here.
Also remove now-unnecessary assertions.
Because we aren't setting these values anymore, these "assertions" aren't doing anything useful.
Instead of trying name + (optional) version and falling back to just name, we just use name outright which can't fail with a version exception.
The single assert in `InstallRequirement.uninstall` covers these cases.
1bd684b
to
e5d4b1c
Compare
Updated to remove some extraneous style changes. |
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.
Quite a lot of commits but it makes the logic behind the change slightly easier to review 👍
Thanks for reviewing! I figured any other way would end up being pretty wordy with a lot of justification or push the burden of checking their equivalence on whoever reviewed. I will squash these since they're only useful for review. |
This is a great PR! Thanks for taking the time to make such a detailed PR! :) |
Previously
InstallRequirement.uninstall
was usingInstallRequirement.check_if_exists
, which is a very overloaded function with several callers that operate at different phases in pip processing, not to mention that it mutatesInstallRequirement
itself.Now we don't use that function for
InstallRequirement.uninstall
.This opens the door to allow scheme-based
pip uninstall
without too much trouble (e.g. #5595). Scheme-basedpip install
pre-upgrade uninstallation will need to come later, but should be simpler because of this refactoring.There should be no behavior change here. I went in very small steps (which should each pass our tests) to hopefully make that easier to verify. This also makes it possible to break off several chunks at a time to validate in individual PRs, if needed.
Please let me know if anything is unclear!