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

Use setup command --regenerate-binstubs option flag #2099

Merged
merged 2 commits into from Nov 27, 2017
Merged

Use setup command --regenerate-binstubs option flag #2099

merged 2 commits into from Nov 27, 2017

Conversation

ghost
Copy link

@ghost ghost commented Nov 24, 2017

Test setup command binstubs regeneration

Since 909b5fb, executable wrappers are regenerated using pristine
command if any gem are installed. New test is inspired by those for
Gem::Commands::PristineCommand.

Fix setup command --regenerate-binstubs option flag

--[no-]regenerate-binstubs option flag was added in 909b5fb but is
not used yet. This change tests if the option was set before calling
Gem::Commands::SetupCommand#regenerate_binstubs.

We also simplify how the option is set, since it's more similar to
format_executable for example (simple option flag), rather than
document where we use the same hash key for multiple options. This
way we can just test the value being either true or false, instead of
testing key presence (Hash#key?) or relying on nil being returned
for nonexistent hash keys with Hash#[].

@ghost
Copy link
Author

ghost commented Nov 24, 2017

Tasks:

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

Let me know if I should adapt those patches. To be honest the test
part is a bit of cargo cult programming inspired by
Gem::Commands::PristineCommand tests, so I would be happy to improve
those and receive any feedback.

Depends on #2098

This build will fail will following error:

OptionParser::InvalidOption: invalid option: --silent

Because if we run Gem::Commands::SetupCommand in tests, we must
also execute Gem::Commands::PristineCommand with
--all --only-executables --silent options, and the last one does not
exist anymore at this time.

@ghost ghost changed the title Commands setup regenerate binstubs Use setup command --regenerate-binstubs option flag Nov 24, 2017
Thibault Jouan added 2 commits November 25, 2017 17:19
  Since 909b5fb, executable wrappers are regenerated using pristine
command if any gem are installed. New test is inspired by those for
Gem::Commands::PristineCommand.
  `--[no-]regenerate-binstubs' option flag was added in 909b5fb but is
not used yet. This change tests if the option was set before calling
Gem::Commands::SetupCommand#regenerate_binstubs.

  We also simplify how the option is set, since it's more similar to
`format_executable' for example (simple option flag), rather than
`document' where we use the same hash key for multiple options. This way
we can just test the value being either true or false, instead of
testing key presence (`Hash#key?') or relying on `nil' being returned
for nonexistent hash keys with `Hash#[]'.
@ghost
Copy link
Author

ghost commented Nov 25, 2017

Depends on #2098

This build will fail will following error:

OptionParser::InvalidOption: invalid option: --silent

Because if we run Gem::Commands::SetupCommand in tests, we must
also execute Gem::Commands::PristineCommand with
--all --only-executables --silent options, and the last one does not
exist anymore at this time.

#2098 was merged, build is now OK.

Please let me know if I should improve anything in this PR
(especially the test part, as said I took inspiration from tests for
pristine command, but I'm not yet familiar with the whole test suite).

@ghost
Copy link
Author

ghost commented Nov 27, 2017

@segiddins, thank you for the review!

I'm still working on changes for setup command, and I'm now wondering
if --[no-]regenerate-binstubs shouldn't be actually named
--[no-]regenerate-wrappers?

Because if I'm correct, it's about RubyGems executable wrappers, and
binstubs seem like something specific to Bundler:
http://bundler.io/man/bundle-binstubs.1.html

I understand that Bundler features are merged into RubyGems, but both
concepts seem slightly different, binstubs will force loading specific
versions for gems if I understand correctly.

Am I wrong? If not, should I open an issue or suggest a PR?

Thanks.

@segiddins
Copy link
Member

That's a good point, and I'm not sure what the answer is? Let's keep it the way it is for now

@hsbt
Copy link
Member

hsbt commented Nov 27, 2017

@bundlerbot r+

@bundlerbot
Copy link
Collaborator

📌 Commit 4619f13 has been approved by hsbt

@bundlerbot
Copy link
Collaborator

⌛ Testing commit 4619f13 with merge d57c530...

bundlerbot added a commit that referenced this pull request Nov 27, 2017
Use setup command --regenerate-binstubs option flag

Test setup command binstubs regeneration
----------------------------------------

  Since 909b5fb, executable wrappers are regenerated using pristine
command if any gem are installed. New test is inspired by those for
`Gem::Commands::PristineCommand`.

Fix setup command --regenerate-binstubs option flag
---------------------------------------------------

  `--[no-]regenerate-binstubs` option flag was added in 909b5fb but is
not used yet. This change tests if the option was set before calling
`Gem::Commands::SetupCommand#regenerate_binstubs`.

  We also simplify how the option is set, since it's more similar to
`format_executable` for example (simple option flag), rather than
`document` where we use the same hash key for multiple options. This
way we can just test the value being either true or false, instead of
testing key presence (`Hash#key?`) or relying on `nil` being returned
for nonexistent hash keys with `Hash#[]`.
@bundlerbot
Copy link
Collaborator

☀️ Test successful - status-travis
Approved by: hsbt
Pushing d57c530 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants