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

Deprecate --install flag to bundle remove and trigger install by default #4891

Merged
merged 1 commit into from
Sep 3, 2021

Conversation

ceritium
Copy link
Contributor

@ceritium ceritium commented Sep 1, 2021

Closes #4889

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

bundle remove does not update lockfile.

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

  • Trigger install command by default on bundle remove
  • Remove option --install from the cli

@ceritium ceritium marked this pull request as ready for review September 1, 2021 16:57
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.

Nice job! My only concern here, is that to be nice citizens we should deprecate the --install flag instead of abruptly removing it.

@ceritium
Copy link
Contributor Author

ceritium commented Sep 2, 2021

@deivid-rodriguez I agree with you, then we should:

  1. Accept the --install argument and print a deprecation warning
  2. Trigger the "install command" by default OR keep the existing behavior and only install by default when we eventually remove the --install argument?
  3. Decide when finally remove the --install. Do you think that Bundler 3 is appropriate, or can we do it earlier? I think that there is no hurry.

What do you think?

@deivid-rodriguez
Copy link
Member

I'd go with

Trigger the "install command" by default

And in the deprecation warning tell the user to not use it, since it's the default behaviour of bundle remove.

Decide when finally remove the --install. Do you think that Bundler 3 is appropriate, or can we do it earlier? I think that there is no hurry.

Yes, we can remove it in bundler 3.

@ceritium ceritium force-pushed the bundle-remove-install branch 3 times, most recently from 2485947 to 050b27e Compare September 2, 2021 19:01
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.

Just a couple of minor comments, but other than that looks good!

bundler/spec/commands/remove_spec.rb Outdated Show resolved Hide resolved
bundler/spec/remove/install_spec.rb Outdated Show resolved Hide resolved
@ceritium
Copy link
Contributor Author

ceritium commented Sep 3, 2021

@deivid-rodriguez, do you think it makes sense to add a pending test for bundler 3 like bundle show --outdated

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.

One final nitpick 😝

bundler/spec/commands/remove_spec.rb Outdated Show resolved Hide resolved
@deivid-rodriguez
Copy link
Member

@deivid-rodriguez, do you think it makes sense to add a pending test for bundler 3 like bundle show --outdated

Sure, yeah!

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.

👍

@deivid-rodriguez deivid-rodriguez changed the title Trigger install command by default on remove Deprecate --install flag to bundle remove and trigger install by default Sep 3, 2021
@ceritium
Copy link
Contributor Author

ceritium commented Sep 3, 2021

@deivid-rodriguez Do you need me to squash the commits or can you do it on merge?

@deivid-rodriguez
Copy link
Member

Actually, if you can do that it'd be awesome! I suspect our backport scripts require explicit merge commits, so we have "squash and merge" disabled.

@deivid-rodriguez deivid-rodriguez merged commit a5f5d32 into rubygems:master Sep 3, 2021
@deivid-rodriguez
Copy link
Member

Thanks!

@ceritium
Copy link
Contributor Author

ceritium commented Sep 4, 2021

@deivid-rodriguez thanks to you. I learn a lot about how the test suite works inside bundler.

@ceritium ceritium deleted the bundle-remove-install branch September 4, 2021 10:57
deivid-rodriguez added a commit that referenced this pull request Sep 22, 2021
Deprecate `--install` flag to `bundle remove` and trigger install by default

(cherry picked from commit a5f5d32)
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.

bundle remove does not update lockfile
3 participants