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

chore: bundle install with --with option is dispensable in this case #629

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sato11
Copy link
Contributor

@sato11 sato11 commented Jan 17, 2023

TLDR; bundle install and bundle install --with development both install the same set. So perhaps one without flag is clearer to use?

The setup script and the ci workflow definition specify that bundler should use --with development option, which has been deprecated as of newer version of bundler. Mine is 2.4.3.

[DEPRECATED] The `--with` flag is deprecated because it relies on being remembered across bundler invocations, which bundler will no longer do in future versions. Instead please use `bundle config set --local with 'development'`, and stop using this flag

While the intention is apparently to restrict installed gems to development group, it makes no difference to the resulting installation set.

This is because we cannot rely on --with to opt out from installing grouped gems. What we might want to use is either --without option or group :test, optional: true. A relevant reference is here:
https://bundler.io/guides/groups.html#optional-groups-and-bundlewith

Here I propose that we drop the irrelevant flag. The discussion above in turn shows that gem 'simplecov' does not need to reside in test group, as it's been, and will be, installed regardless of the presence of --with. Therefore it's got rid of as well.

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

If you have questions, please file a support ticket, or create a GitHub Issue in this repository.

The setup script and the ci workflow definition specify that bundler should use
`--with development` option, which has been deprecated as of newer version of bundler.

While the intention apparently is to restrict installed gems to development group,
it makes no difference to the resulting installation set.

This is because we cannot rely on `--with` to opt out from installing grouped gems.
What we might want to use is either `--without` option or `group :test, optional: true`.
A relevant reference is here:
https://bundler.io/guides/groups.html#optional-groups-and-bundlewith

The discussion above in turn shows that `gem 'simplecov'` does not need to
reside in test group, as it's been, and will be, installed regardless of
the presence of `--with`. Therefore it's got rid of as well.
@shrutiburman
Copy link
Contributor

Thanks for the PR! We've added the review to our backlog to be prioritised. Pull requests and +1s on the issue summary will help it move up the backlog.

@shrutiburman shrutiburman added type: twilio enhancement feature request on Twilio's roadmap status: waiting for feature feature will be implemented in the future labels Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for feature feature will be implemented in the future type: twilio enhancement feature request on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants