Correct `bundle show` deprecation #6718

deivid-rodriguez · 2018-10-02T00:28:50Z

What was the end-user problem that led to this PR?

The problem was bundle show deprecation messages are incorrect:

$ bundle show yard
[DEPRECATED FOR 2.0] use `bundle list` instead of `bundle show`
Resolving dependencies...
/home/deivid/Code/activeadmin/.bundle/ruby/2.5.0/gems/yard-0.9.16
$ bundle list yard
ERROR: "bundle list" was called with arguments ["yard"]
Usage: "bundle list"

What was your diagnosis of the problem?

My diagnosis was that deprecation messages only mention bundle list, but in some cases, the replacement is bundle info.

What is your fix for the problem, implemented in this PR?

My fix is to replace "show" in the original command with the appropriate alternative in each case.

Why did you choose this fix out of the possible options?

I chose this fix because it was the most user friendly message, since it prints the exact command the user needs to type to get rid of the warning.

I don't know why it was there, and it makes things more complicated with dealing and running assertions on ARGV.

With a single gem, it will be replaced by `bundle info`, not by `bundle list`.

segiddins · 2018-10-03T04:35:35Z

spec/bundler/cli_spec.rb

@@ -71,17 +71,17 @@
    it "prints the running command" do
      gemfile ""
      bundle! "info bundler", :verbose => true
-      expect(last_command.stdout).to start_with("Running `bundle info bundler --no-color --verbose` with bundler #{Bundler::VERSION}")
+      expect(last_command.stdout).to start_with("Running `bundle info bundler --verbose` with bundler #{Bundler::VERSION}")


why did this change?

Because it's required by the removal of the inconditional --no-color addition that the bundle! helper makes. I don't understand it, and it makes no sense to me. Here we are esentially testing that if we run the command bundle info bundler --verbose, we should see

Running `bundle info bundler --no-color --verbose` with bundler #{Bundler::VERSION}".

Why the --no-color part? That does not happen is real usage, does it?

segiddins · 2018-10-03T04:36:47Z

spec/support/helpers.rb

@@ -99,9 +99,6 @@ def bundle(cmd, options = {})
      with_sudo = options.delete(:sudo)
      sudo = with_sudo == :preserve_env ? "sudo -E" : "sudo" if with_sudo

-      no_color = options.delete("no-color") { cmd.to_s !~ /\A(e|ex|exe|exec|conf|confi|config)(\s|\z)/ }
-      options["no-color"] = true if no_color


Why did you remove this?

I was trying to test the command line arguments that bundle show receives and for some reason I was getting the --no-color flag added to them? Then I found this dance and I added s|sh|sho|show as exceptions here and it worked.

But then I decided that doing this didn't make any sense to me, so I removed it so I could get some feedback from you 😃. Why are we doing that?

So all the test output will never contain ANSI escape codes , and thus be consistent no matter how the tests are being run

Can you show an example of this inconsistency? Our current test environment seems to handle this just fine? (I think the explanation of why this just works is that an open3 context is recognized by ruby as a "non-tty" situation, so it's automatically turned off by bundler).

To me doing this is not good because the helper does not represent real world usage, but a slight modification of it, and it leads to weird assertions (like the ones I'm fixing here), and suprising behavior for the developer (when I first saw this I was like "what? where did this flag come from?", and I had to spend a while tracking it down and adding yet more exceptions here).

You could also pass no-color => false to the bundle helper

Right. So is that what you're asking me to do?

I'm going to do that and make my case to remove this separately 🙂.

This flag will be needed on most bundle show specs. Do you prefer that I explicitly pass the flag to every bundle invocation that needs it, that I add more exceptions inside the method, or that I leave this patch as it is? (I'm asking because I just noticed that you approved this PR, so I'm not sure how to proceed)

It's your call :) If it doesn't break the tests, I suppose removing it is fine?

Ok, yeah, I think it's fine!

deivid-rodriguez · 2018-10-06T11:49:07Z

@bundlerbot r+

6718: Correct `bundle show` deprecation r=deivid-rodriguez a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was `bundle show` deprecation messages are incorrect: ``` $ bundle show yard [DEPRECATED FOR 2.0] use `bundle list` instead of `bundle show` Resolving dependencies... /home/deivid/Code/activeadmin/.bundle/ruby/2.5.0/gems/yard-0.9.16 $ bundle list yard ERROR: "bundle list" was called with arguments ["yard"] Usage: "bundle list" ``` ### What was your diagnosis of the problem? My diagnosis was that deprecation messages only mention `bundle list`, but in some cases, the replacement is `bundle info`. ### What is your fix for the problem, implemented in this PR? My fix is to replace "show" in the original command with the appropriate alternative in each case. ### Why did you choose this fix out of the possible options? I chose this fix because it was the most user friendly message, since it prints the exact command the user needs to type to get rid of the warning. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

ghost · 2018-10-06T12:16:02Z

Build succeeded

continuous-integration/travis-ci/push

6718: Correct `bundle show` deprecation r=deivid-rodriguez a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was `bundle show` deprecation messages are incorrect: ``` $ bundle show yard [DEPRECATED FOR 2.0] use `bundle list` instead of `bundle show` Resolving dependencies... /home/deivid/Code/activeadmin/.bundle/ruby/2.5.0/gems/yard-0.9.16 $ bundle list yard ERROR: "bundle list" was called with arguments ["yard"] Usage: "bundle list" ``` ### What was your diagnosis of the problem? My diagnosis was that deprecation messages only mention `bundle list`, but in some cases, the replacement is `bundle info`. ### What is your fix for the problem, implemented in this PR? My fix is to replace "show" in the original command with the appropriate alternative in each case. ### Why did you choose this fix out of the possible options? I chose this fix because it was the most user friendly message, since it prints the exact command the user needs to type to get rid of the warning. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net> (cherry picked from commit 703b663)

* 1-17-stable: (38 commits) Version 1.17.0 with changelog Merge #6754 Version 1.17.0.pre.2 with changelog fix failing bundle remove specs Still document the `--force` option scope specs testing bundler 2 deprecations to bundler 1 only Merge #6718 Merge #6707 Merge #6702 Merge #6316 Auto merge of #6447 - agrim123:agr-update-error-message, r=segiddins Auto merge of #6513 - agrim123:agr-bundler-remove, r=indirect Auto merge of #6318 - jhawthorn:fix_incorrect_test_in_requires_sudo, r=segiddins Auto merge of #6450 - bundler:segiddins/bundle-binstubs-all, r=colby-swandale Auto merge of #6024 - gwerbin:custom-user-bundle-dir, r=colby-swandale Version 1.17.0.pre.1 with changelog Auto merge of #5964 - bundler:colby/deprecate-viz-command, r=segiddins Auto merge of #5986 - bundler:seg-jobs-count, r=indirect Auto merge of #5995 - bundler:seg-gvp-major, r=indirect Auto merge of #5803 - igorbozato:path_relative_to_pwd, r=indirect ...

deivid-rodriguez added 4 commits October 1, 2018 21:26

Remove "no-color" tweaks from specs

9deb544

I don't know why it was there, and it makes things more complicated with dealing and running assertions on ARGV.

Fix "bundle show" deprecation messages

fefd4c9

With a single gem, it will be replaced by `bundle info`, not by `bundle list`.

Fix --paths argument handling

2dd62a3

Refactor initial code

fd9adf0

segiddins approved these changes Oct 3, 2018

View reviewed changes

ghost merged commit fd9adf0 into master Oct 6, 2018

deivid-rodriguez deleted the correct_bundle_show_deprecation branch October 6, 2018 13:11

deivid-rodriguez added this to the 1.17.0 milestone Oct 6, 2018

colby-swandale modified the milestone: 1.17.0 Oct 11, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct `bundle show` deprecation #6718

Correct `bundle show` deprecation #6718

deivid-rodriguez commented Oct 2, 2018

segiddins Oct 3, 2018

deivid-rodriguez Oct 3, 2018 •

edited

segiddins Oct 3, 2018

deivid-rodriguez Oct 3, 2018 •

edited

segiddins Oct 3, 2018

deivid-rodriguez Oct 3, 2018

segiddins Oct 3, 2018

deivid-rodriguez Oct 3, 2018

deivid-rodriguez Oct 3, 2018

deivid-rodriguez Oct 3, 2018 •

edited

segiddins Oct 6, 2018

deivid-rodriguez Oct 6, 2018

deivid-rodriguez commented Oct 6, 2018

ghost commented Oct 6, 2018

Correct bundle show deprecation #6718

Correct bundle show deprecation #6718

Conversation

deivid-rodriguez commented Oct 2, 2018

What was the end-user problem that led to this PR?

What was your diagnosis of the problem?

What is your fix for the problem, implemented in this PR?

Why did you choose this fix out of the possible options?

Choose a reason for hiding this comment

deivid-rodriguez Oct 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deivid-rodriguez Oct 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deivid-rodriguez Oct 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deivid-rodriguez commented Oct 6, 2018

ghost commented Oct 6, 2018

Build succeeded

Correct `bundle show` deprecation #6718

Correct `bundle show` deprecation #6718

deivid-rodriguez Oct 3, 2018 •

edited

deivid-rodriguez Oct 3, 2018 •

edited

deivid-rodriguez Oct 3, 2018 •

edited