Improve gem uninstall --all
#2893
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 tasks
Contributor
Author
|
If we did this, and released it with 3.0.7, we'll probably save some people from having to change some CI scripts. |
deivid-rodriguez
force-pushed
the
gem_uninstall_all
branch
2 times, most recently
from
a502ff5 to
534df70
Compare
benoittgt
reviewed
Aug 21, 2019
| end | ||
|
|
||
| if list.empty? | ||
| raise Gem::InstallError, "gem #{@gem.inspect} is not installed" |
There was a problem hiding this comment.
Don't you think we could instead have an error class Gem::UninstallError when we do uninstall?
It is probably a breaking change... :/
Contributor
Author
There was a problem hiding this comment.
Absolutely, it was very confusing the first time I saw this. I'll try to change this in the future.
|
Thanks a lot for the quick PR @deivid-rodriguez and @hsbt for the review. 🙏 |
bronzdoc
suggested changes
Aug 23, 2019
Instead, display an informative message saying that uninstallation of specific versions is being skipped because of being default gems.
deivid-rodriguez
force-pushed
the
gem_uninstall_all
branch
from
534df70 to
b44845a
Compare
bronzdoc
approved these changes
Aug 27, 2019
Contributor
bronzdoc
left a comment
bronzdoc
left a comment
There was a problem hiding this comment.
LGTM @deivid-rodriguez !
Contributor
|
@bundlerbot r+ |
ghost
pushed a commit
that referenced
this pull request
Aug 27, 2019
2893: Improve `gem uninstall --all` r=bronzdoc a=deivid-rodriguez # Description: After we started unswallowing errors of `gem uninstall` in #2707, some people got failing CI runs because they were doing things like `gem uninstall <default_gem> --all --force`. Previously that would work just fine, because the error while trying to uninstall a default gem was being swallowed. In my opinion, if you try to do `gem uninstall bundler:1.17.2` (on a ruby where this is the default bundler version), then it is fine to fail hard, but if you do `gem uninstall bundler --all`, then the command should uninstall all bundler versions that can be uninstalled (and skip the default version), but not fail just because bundler is a default gem. It can be argued though that the command should indeed fail hard, because it didn't really succeeded on trying to do what the user asked for: the user tried to uninstall _all_versions, but that wasn't achieved because the default version was left around. In order to find a compromise between these two visions, and maximize usability, I propose to not fail hard, but log a line for each default version that couldn't be removed. Note that the current behavior is completely inconsistent (in my opinion), because it fails if the default copy is the single copy in the system, but succeeds if it's not: ``` $ gem uninstall rdoc --all --force ERROR: While executing gem ... (Gem::InstallError) gem "rdoc" cannot be uninstalled because it is a default gem $ gem install rdoc Fetching rdoc-6.1.1.gem Successfully installed rdoc-6.1.1 1 gem installed $ gem uninstall rdoc --all --force Successfully uninstalled rdoc-6.1.1 ``` After this PR, the messages are more consistent and informative, in my opinion, and the status code is always success (as it was before #2707). ``` $ ruby -Ilib bin/gem uninstall rdoc --all --force Ignored rdoc-6.1.0 because it is a default gem $ gem install rdoc Fetching rdoc-6.1.1.gem Successfully installed rdoc-6.1.1 1 gem installed $ ruby -Ilib bin/gem uninstall rdoc --all --force Ignored rdoc-6.1.0 because it is a default gem Successfully uninstalled rdoc-6.1.1 ``` Thoughts? # Tasks: - [x] Describe the problem / feature - [x] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Closes #2892. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
Build succeeded |
ghost
merged commit
aried3r
added a commit
to rpush/rpush
that referenced
this pull request
Sep 4, 2019
A change in rubygems [0] made our CI fail, but since it was every run, it showed that this line was indeed not needed. A fix in a future rubygems version [1] should fix this as well, but I think this is the way to go. [0] ruby/rubygems#2707 [1] ruby/rubygems#2893
Adrian1707
pushed a commit
to Adrian1707/rpush
that referenced
this pull request
Apr 24, 2024
A change in rubygems [0] made our CI fail, but since it was every run, it showed that this line was indeed not needed. A fix in a future rubygems version [1] should fix this as well, but I think this is the way to go. [0] ruby/rubygems#2707 [1] ruby/rubygems#2893
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
After we started unswallowing errors of
gem uninstallin #2707, some people got failing CI runs because they were doing things likegem uninstall <default_gem> --all --force. Previously that would work just fine, because the error while trying to uninstall a default gem was being swallowed.In my opinion, if you try to do
gem uninstall bundler:1.17.2(on a ruby where this is the default bundler version), then it is fine to fail hard, but if you dogem uninstall bundler --all, then the command should uninstall all bundler versions that can be uninstalled (and skip the default version), but not fail just because bundler is a default gem.It can be argued though that the command should indeed fail hard, because it didn't really succeeded on trying to do what the user asked for: the user tried to uninstall _all_versions, but that wasn't achieved because the default version was left around.
In order to find a compromise between these two visions, and maximize usability, I propose to not fail hard, but log a line for each default version that couldn't be removed.
Note that the current behavior is completely inconsistent (in my opinion), because it fails if the default copy is the single copy in the system, but succeeds if it's not:
After this PR, the messages are more consistent and informative, in my opinion, and the status code is always success (as it was before #2707).
Thoughts?
Tasks:
I will abide by the code of conduct.
Closes #2892.