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

Fix deprecation messages for bundle install flags, the config should be --local as before #3917

Conversation

eregon
Copy link
Contributor

@eregon eregon commented Aug 30, 2020

Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Looks good! Being explicit is a good step forward for now, and makes it easier to change the default in the future.

bundler/spec/commands/outdated_spec.rb Outdated Show resolved Hide resolved
bundler/lib/bundler/fetcher.rb Show resolved Hide resolved
@eregon eregon force-pushed the fix-deprecation-warning-that-would-wrongly-config-globally branch from d6a4a87 to aac87f3 Compare August 30, 2020 14:55
@eregon eregon force-pushed the fix-deprecation-warning-that-would-wrongly-config-globally branch from aac87f3 to eff9bba Compare August 30, 2020 15:01
@eregon eregon force-pushed the fix-deprecation-warning-that-would-wrongly-config-globally branch from eff9bba to fed953c Compare August 30, 2020 15:05
@eregon
Copy link
Contributor Author

eregon commented Aug 30, 2020

@deivid-rodriguez I think I addressed everything now, and I added a CHANGELOG entry, should be good to go, except that regeneration of bundle-config.1.txt (unless we're fine with it?).

@eregon
Copy link
Contributor Author

eregon commented Aug 30, 2020

FWIW I like bundle config --local path vendor/bundle better than bundle config set --local path vendor/bundle but given that seems deprecated for Bundler 3 I went with the Bundler 3 syntax and explicit set (which is another difference to git config).
(Specifically if we change the default to --local, bundle config path vendor/bundle sounds nicer than bundle config set path vendor/bundle to me)

@deivid-rodriguez
Copy link
Member

@deivid-rodriguez I think I addressed everything now, and I added a CHANGELOG entry, should be good to go, except that regeneration of bundle-config.1.txt (unless we're fine with it?).

You can skip the changelog message since I plan to start generating the changelog automatically from merge PR titles and labels in the next version.

FWIW I like bundle config --local path vendor/bundle better than bundle config set --local path vendor/bundle but given that seems deprecated for Bundler 3 I went with the Bundler 3 syntax and explicit set (which is another difference to git config).
(Specifically if we change the default to --local, bundle config path vendor/bundle sounds nicer than bundle config set path vendor/bundle to me)

I'm also unsure about deprecating the old bundle config interface, that's why I delayed it until it can be better discussed, and the original reasons properly reviewed: rubygems/bundler#7475. But we're using the new interface internally in a consistent way at the moment, so let's keep doing it for the moment 👍.

@deivid-rodriguez
Copy link
Member

Also, I think we can ignore the annoying txt diff for this PR, and try to get rid of txt files at all as you suggested.

@eregon
Copy link
Contributor Author

eregon commented Aug 30, 2020

You can skip the changelog message since I plan to start generating the changelog automatically from merge PR titles and labels in the next version.

Should I remove that change then?

I'm also unsure about deprecating the old bundle config interface, that's why I delayed it until it can be better discussed, and the original reasons properly reviewed: rubygems/bundler#7475. But we're using the new interface internally in a consistent way at the moment, so let's keep doing it for the moment .

Is there some good place to discuss that? I feel this deprecation is going to be rather annoying (and require verbose commands) for very little gains. The only conflict would be if one needs to set a key named list/get/set/unset but that seems extremely unlikely.

Also, I think we can ignore the annoying txt diff for this PR, and try to get rid of txt files at all as you suggested.

Will you make a PR or is there a plan for that then? 😃
I guess we should wait to merge this PR after that?

I think this PR should be merged for the next Bundler 2 release, it's quite bad if people configure globally when they do not intend to (can cause confusion, or an impression that the deprecation messages are just broken and due to that might be ignored more).

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Aug 30, 2020

Should I remove that change then?

It will be overwritten unless the PR title matches the current wording exactly, so yeah I'd rather remove it!

Is there some good place to discuss that? I feel this deprecation is going to be rather annoying (and require verbose commands) for very little gains. The only conflict would be if one needs to set a key named list/get/set/unset but that seems extremely unlikely.

Feel free to open a new ticket, investigate why the new interface was added, and explain why you think the reasons for making it compulsory are wrong.

Will you make a PR or is there a plan for that then? 😃
I guess we should wait to merge this PR after that?

I like your idea, but it came up literally 1 hour ago, so I'm not sure when I (or anybody) will get to it. Better not to block this PR on that.

I think this PR should be merged for the next Bundler 2 release, it's quite bad if people configure globally when they do not intend to (can cause confusion, or an impression that the deprecation messages are just broken and due to that might be ignored more).

Sure, just be patient! :)

…d be --local as before

* See rubygems#3916
* Always show --local or --global for `bundle config set` commands as
  the default is global which can be not intuitive (rubygems#3916).
@eregon eregon force-pushed the fix-deprecation-warning-that-would-wrongly-config-globally branch from fed953c to ff64295 Compare August 30, 2020 16:24
@eregon
Copy link
Contributor Author

eregon commented Aug 30, 2020

OK, CHANGELOG entry removed. I worked around and now the diff for bundle-config.1.txt should be minimal.
Ready to be merged :)

@deivid-rodriguez
Copy link
Member

Thanks!

@deivid-rodriguez deivid-rodriguez merged commit dc133dc into rubygems:master Sep 2, 2020
deivid-rodriguez added a commit that referenced this pull request Oct 5, 2020
…ld-wrongly-config-globally

Fix deprecation messages for `bundle install` flags, the config should be --local as before

(cherry picked from commit dc133dc)
deivid-rodriguez added a commit that referenced this pull request Oct 5, 2020
…ld-wrongly-config-globally

Fix deprecation messages for `bundle install` flags, the config should be --local as before

(cherry picked from commit dc133dc)
deivid-rodriguez added a commit that referenced this pull request Oct 5, 2020
…ld-wrongly-config-globally

Fix deprecation messages for `bundle install` flags, the config should be --local as before

(cherry picked from commit dc133dc)
deivid-rodriguez added a commit that referenced this pull request Oct 6, 2020
…ld-wrongly-config-globally

Fix deprecation messages for `bundle install` flags, the config should be --local as before

(cherry picked from commit dc133dc)
deivid-rodriguez added a commit that referenced this pull request Oct 6, 2020
…ld-wrongly-config-globally

Fix deprecation messages for `bundle install` flags, the config should be --local as before

(cherry picked from commit dc133dc)
deivid-rodriguez added a commit that referenced this pull request Oct 6, 2020
…ld-wrongly-config-globally

Fix deprecation messages for `bundle install` flags, the config should be --local as before

(cherry picked from commit dc133dc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecation messages recommending bundle config don't have the same effect as the old persisted flags
3 participants