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 typo "c" -> "user_config" for --extra_runtime_dependencies #2050

Merged

Conversation

olleolleolle
Copy link
Contributor

@olleolleolle olleolleolle commented Oct 23, 2019

Description

The CLI option parsing code for --extra-runtime-dependencies had a typo in it.

There was a c here, and it seemed, out of place.

o.on "--extra-runtime-dependencies GEM1,GEM2", "Defines any extra needed gems when using --prune-bundler" do |arg|
  c.extra_runtime_dependencies arg.split(',')
end

Comparing with --prune_bundler, which was mentioned next to this long option, we could try using user_config instead.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] to all commit messages.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the commit messages.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

olleolleolle added a commit to olleolleolle/puma that referenced this pull request Oct 23, 2019
@olleolleolle
Copy link
Contributor Author

@nateberkopec I inspected this, and just was unable to locate c.

I compared with user_config.prune_bundler and picked user_config for this.

@olleolleolle olleolleolle marked this pull request as ready for review October 23, 2019 17:14
Copy link
Contributor

@composerinteralia composerinteralia left a comment

Choose a reason for hiding this comment

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

This looks similar to #2039. We got rid of c in cda9317, but the PR to add this option (#1105) was opened before that change was made and the branch was probably never updated.

I will search for any more lingering references to c that are broken.

@nateberkopec
Copy link
Member

Needs a test 👍

olleolleolle added a commit to olleolleolle/puma that referenced this pull request Oct 24, 2019
@olleolleolle olleolleolle force-pushed the extra-runtime-dependencies-setting branch from 82aae60 to 13dc698 Compare October 24, 2019 08:00
@olleolleolle
Copy link
Contributor Author

(I added the simplest test for this code path - something that would have failed with the old code.)

test/test_cli.rb Outdated Show resolved Hide resolved
test/test_cli.rb Outdated Show resolved Hide resolved
@olleolleolle olleolleolle force-pushed the extra-runtime-dependencies-setting branch from 13dc698 to b286f7e Compare October 25, 2019 11:18
@nateberkopec nateberkopec merged commit 122e08f into puma:master Oct 25, 2019
@nateberkopec
Copy link
Member

Thanks @olleolleolle 🎉

This was referenced Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants