Skip to content

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
Contributor

@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
Contributor

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
Contributor

@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!

@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
Contributor

@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 😝

@deivid-rodriguez
Copy link
Contributor

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

Sure, yeah!

Copy link
Contributor

@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
Contributor

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.

@ceritium ceritium force-pushed the bundle-remove-install branch from 99ad588 to 9c452d9 Compare September 3, 2021 12:53
@ceritium ceritium force-pushed the bundle-remove-install branch from 9c452d9 to 2b17544 Compare September 3, 2021 12:55
@deivid-rodriguez deivid-rodriguez merged commit a5f5d32 into ruby:master Sep 3, 2021
@deivid-rodriguez
Copy link
Contributor

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