-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add message for gem/bundle upgrade to point users to gem/bundle update
#9407
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -185,6 +185,12 @@ def help(cli = nil) | |
| end | ||
|
|
||
| def self.handle_no_command_error(command, has_namespace = $thor_runner) | ||
| if command == "upgrade" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'd have to move this down below (L199), otherwise this is going to break plugins. Bundler has a plugin system where the CLI can be extended to offer more commands. I don't know if there is already a plugin in the wild that provides
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I called this out in a comment: #9407 (comment) You mention this in your review's main message:
If we do want to reserve This is also why I didn't use an alias. Since this PR implements this as an error message, it would not be a breaking change to swap out the error message and put in real behaviour. By comparison if
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oops sorry I missed that.
I agree, but it is a breaking change now. There is probably a plugin in the wild that offer a |
||
| command = Bundler.ui.add_color("bundle update <gem_name>", :bold, :cyan) | ||
| Bundler.ui.info("Please use #{command} to update gems in your bundle.") | ||
| exit(1) | ||
| end | ||
Edouard-chin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if Bundler.settings[:plugins] && Bundler::Plugin.command?(command) | ||
| return Bundler::Plugin.exec_command(command, ARGV[1..-1]) | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -26,6 +26,44 @@ def self.attach_correctable | |||||
| DidYouMean.correct_error(Gem::UnknownCommandError, Gem::UnknownCommandSpellChecker) | ||||||
| end | ||||||
| end | ||||||
|
|
||||||
| def detailed_message(highlight: true, **) | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried implementing this by modifying the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I could do it there, but improve the formatting of all such messages with a |
||||||
| msg = super(highlight: highlight) | ||||||
|
|
||||||
| case unknown_command | ||||||
| when "upgrade" | ||||||
| command = "bundle update <gem_name>" | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| bold = "\e[1m" | ||||||
| cyan = "\e[36m" | ||||||
| reset = "\e[0m" | ||||||
|
|
||||||
| command = if highlight && can_display_colors? | ||||||
| "#{bold}#{cyan}#{command}#{reset}" | ||||||
| else | ||||||
| "`#{command}`" | ||||||
| end | ||||||
|
|
||||||
| msg + "\nPlease use #{command} to update gems in your bundle." | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||||||
|
|
||||||
| else | ||||||
| msg | ||||||
| end | ||||||
| end | ||||||
|
|
||||||
| private | ||||||
|
|
||||||
| def can_display_colors? | ||||||
| are_colors_supported? && !are_colors_disabled? | ||||||
| end | ||||||
|
|
||||||
| def are_colors_supported? | ||||||
| stdout.tty? && ENV["TERM"] != "dumb" | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| end | ||||||
|
|
||||||
| def are_colors_disabled? | ||||||
| !ENV["NO_COLOR"].nil? && !ENV["NO_COLOR"].empty? | ||||||
| end | ||||||
| end | ||||||
|
|
||||||
| class Gem::DependencyError < Gem::Exception; end | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -91,6 +91,24 @@ | |
| assert_equal message, actual_message | ||
| end | ||
|
|
||
| def test_upgrade_points_users_to_update | ||
| e = assert_raise Gem::UnknownCommandError do | ||
| @command_manager.find_command "upgrade" | ||
| end | ||
|
|
||
| assert_equal <<~MSG.chomp, e.detailed_message(highlight: false) | ||
| Unknown command upgrade (Gem::UnknownCommandError) | ||
| Please use `bundle update <gem_name>` to update gems in your bundle. | ||
| MSG | ||
|
|
||
| def e.can_display_colors? = true | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we don't need this in test ? I find difficult to read the assertion otherwise. |
||
|
|
||
| assert_equal <<~MSG.chomp, e.detailed_message(highlight: true) | ||
|
Check failure on line 106 in test/rubygems/test_gem_command_manager.rb
|
||
| \e[1mUnknown command upgrade (\e[1;4mGem::UnknownCommandError\e[m\e[1m)\e[m | ||
| Please use \e[1m\e[36mbundle update <gem_name>\e[0m to update gems in your bundle. | ||
| MSG | ||
| end | ||
|
|
||
| def test_run_interrupt | ||
| old_load_path = $:.dup | ||
| $: << File.expand_path("test/rubygems", PROJECT_DIR) | ||
|
|
||
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 put this check above the check for plugins below, so that would prevent having a plugin named "upgrade". That seems desirable to me, but what do you think?