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

In 'readall.rb', replaced multi-step 'each' loop with one-line method chain of Ruby enumerator methods #3304

Merged
merged 6 commits into from Oct 20, 2017

Conversation

Projects
None yet
4 participants
@richiethomas
Contributor

richiethomas commented Oct 11, 2017

Combine the two RegExp calls that are performed on each filename in readall.rb, reducing the # of checks performed by 50%.

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
    N/A (refactoring only; no new functionality; existing test suite passes)
  • Have you successfully run brew tests with your changes locally?

Show outdated Hide outdated Library/Homebrew/cmd/readall.rb Outdated
Show outdated Hide outdated Library/Homebrew/cmd/readall.rb Outdated
@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Oct 17, 2017

Member

brew style is failing; please fix it up, thanks.

Member

MikeMcQuaid commented Oct 17, 2017

brew style is failing; please fix it up, thanks.

@richiethomas

This comment has been minimized.

Show comment
Hide comment
@richiethomas

richiethomas Oct 17, 2017

Contributor

@MikeMcQuaid I noticed the brew style failures as well. I attempted to run the command on my local to get more info, and I saw Rubygem failures. Specifically:

Ignoring rainbow-2.2.2 because its extensions are not built.  Try: gem pristine rainbow --version 2.2.2
/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require': cannot load such file -- rainbow (LoadError)`
from /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
from /Users/richie.thomas/.gem/ruby/2.3.0/gems/rubocop-0.50.0/lib/rubocop.rb:4:in `<top (required)>'
from /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
from /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
from /Users/richie.thomas/.gem/ruby/2.3.0/gems/rubocop-0.50.0/bin/rubocop:6:in `<top (required)>'
from /Users/richie.thomas/.gem/ruby/2.3.0/bin/rubocop:22:in `load'
from /Users/richie.thomas/.gem/ruby/2.3.0/bin/rubocop:22:in `<main>'

I suspect the reason is that I manage my local Ruby versions using rbenv, and it looks like Homebrew expects to use the default system Ruby. Is that right? If so, I assume I'm not the first person to encounter this problem. Any known workaround (other than uninstalling rbenv to run brew style)?

Contributor

richiethomas commented Oct 17, 2017

@MikeMcQuaid I noticed the brew style failures as well. I attempted to run the command on my local to get more info, and I saw Rubygem failures. Specifically:

Ignoring rainbow-2.2.2 because its extensions are not built.  Try: gem pristine rainbow --version 2.2.2
/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require': cannot load such file -- rainbow (LoadError)`
from /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
from /Users/richie.thomas/.gem/ruby/2.3.0/gems/rubocop-0.50.0/lib/rubocop.rb:4:in `<top (required)>'
from /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
from /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
from /Users/richie.thomas/.gem/ruby/2.3.0/gems/rubocop-0.50.0/bin/rubocop:6:in `<top (required)>'
from /Users/richie.thomas/.gem/ruby/2.3.0/bin/rubocop:22:in `load'
from /Users/richie.thomas/.gem/ruby/2.3.0/bin/rubocop:22:in `<main>'

I suspect the reason is that I manage my local Ruby versions using rbenv, and it looks like Homebrew expects to use the default system Ruby. Is that right? If so, I assume I'm not the first person to encounter this problem. Any known workaround (other than uninstalling rbenv to run brew style)?

@richiethomas

This comment has been minimized.

Show comment
Hide comment
@richiethomas

richiethomas Oct 17, 2017

Contributor

FWIW I have a StackOverflow question posted here, in case the problem isn't clear from my description.

Contributor

richiethomas commented Oct 17, 2017

FWIW I have a StackOverflow question posted here, in case the problem isn't clear from my description.

@reitermarkus

This comment has been minimized.

Show comment
Hide comment
@reitermarkus

reitermarkus Oct 18, 2017

Member

I'm using rbenv and never had any issues. Did you try gem pristine rainbow --version 2.2.2, like the error suggested?

Member

reitermarkus commented Oct 18, 2017

I'm using rbenv and never had any issues. Did you try gem pristine rainbow --version 2.2.2, like the error suggested?

@richiethomas

This comment has been minimized.

Show comment
Hide comment
@richiethomas

richiethomas Oct 18, 2017

Contributor

@reitermarkus yes, with the following results:

$ gem pristine rainbow --version 2.2.2
Restoring gems to pristine condition...
Building native extensions.  This could take a while...
Restored rainbow-2.2.2

Re-running brew style then produces the same error output:

$ brew style
Ignoring rainbow-2.2.2 because its extensions are not built.  Try: gem pristine rainbow --version 2.2.2
/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require': cannot load such file -- rainbow (LoadError)
from /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
from /Users/richie.thomas/.gem/ruby/2.3.0/gems/rubocop-0.50.0/lib/rubocop.rb:4:in `<top (required)>'
from /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
from /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
from /Users/richie.thomas/.gem/ruby/2.3.0/gems/rubocop-0.50.0/bin/rubocop:6:in `<top (required)>'
from /Users/richie.thomas/.gem/ruby/2.3.0/bin/rubocop:22:in `load'
from /Users/richie.thomas/.gem/ruby/2.3.0/bin/rubocop:22:in `<main>'

The reason I suspect the cause is related to gem management is because of the tail end of the stack trace from running brew style. It references installation paths for the rubocop gem, but not the rbenv installation path mentioned when I run gem environment. The latter command shows an install path of /Users/richie.thomas/.rbenv/versions/2.3.3/lib/ruby/gems/2.3.0, not /Users/richie.thomas/.gem/ruby/2.3.0/.

Contributor

richiethomas commented Oct 18, 2017

@reitermarkus yes, with the following results:

$ gem pristine rainbow --version 2.2.2
Restoring gems to pristine condition...
Building native extensions.  This could take a while...
Restored rainbow-2.2.2

Re-running brew style then produces the same error output:

$ brew style
Ignoring rainbow-2.2.2 because its extensions are not built.  Try: gem pristine rainbow --version 2.2.2
/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require': cannot load such file -- rainbow (LoadError)
from /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
from /Users/richie.thomas/.gem/ruby/2.3.0/gems/rubocop-0.50.0/lib/rubocop.rb:4:in `<top (required)>'
from /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
from /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
from /Users/richie.thomas/.gem/ruby/2.3.0/gems/rubocop-0.50.0/bin/rubocop:6:in `<top (required)>'
from /Users/richie.thomas/.gem/ruby/2.3.0/bin/rubocop:22:in `load'
from /Users/richie.thomas/.gem/ruby/2.3.0/bin/rubocop:22:in `<main>'

The reason I suspect the cause is related to gem management is because of the tail end of the stack trace from running brew style. It references installation paths for the rubocop gem, but not the rbenv installation path mentioned when I run gem environment. The latter command shows an install path of /Users/richie.thomas/.rbenv/versions/2.3.3/lib/ruby/gems/2.3.0, not /Users/richie.thomas/.gem/ruby/2.3.0/.

@reitermarkus

This comment has been minimized.

Show comment
Hide comment
@reitermarkus

reitermarkus Oct 18, 2017

Member

Have you tried rm -r ~/.gem?

Member

reitermarkus commented Oct 18, 2017

Have you tried rm -r ~/.gem?

@richiethomas

This comment has been minimized.

Show comment
Hide comment
@richiethomas

richiethomas Oct 18, 2017

Contributor

@reitermarkus I renamed ~/.gem/ and re-ran brew style, and that did the trick! Thanks!

Contributor

richiethomas commented Oct 18, 2017

@reitermarkus I renamed ~/.gem/ and re-ran brew style, and that did the trick! Thanks!

@@ -39,7 +39,7 @@ def list
filtered_list
elsif ARGV.named.empty?
if ARGV.include? "--full-name"
full_names = Formula.installed.map(&:full_name).sort &tap_and_name_comparison
full_names = Formula.installed.map(&:full_name).sort(&tap_and_name_comparison)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 20, 2017

Member

Please remove this change as it's otherwise unrelated to this PR, thanks!

@MikeMcQuaid

MikeMcQuaid Oct 20, 2017

Member

Please remove this change as it's otherwise unrelated to this PR, thanks!

This comment has been minimized.

@richiethomas

richiethomas Oct 20, 2017

Contributor

This change was actually prompted by a test failure I saw on my local:

  1) brew readall imports all Formulae for a given Tap
     Failure/Error:
       expect { brew "readall", "--aliases", "--syntax" }
         .to be_a_success
         .and not_to_output.to_stdout
         .and not_to_output.to_stderr
 
      expected #<Proc:0x00000104b03ee8@/usr/local/Homebrew/Library/Homebrew/test/cmd/readall_spec.rb:10> to be a success
 
   ...and:
 
      expected block to not output to stderr, but output "/usr/local/Homebrew/Library/Homebrew/cmd/list.rb:42: warning: `&' interpreted as argument prefix\n"
 # ./test/cmd/readall_spec.rb:10:in `block (2 levels) in <top (required)>'
 # ./test/support/helper/spec/shared_context/integration_test.rb:44:in `block (2 levels) in <top (required)>'
 # ./test/spec_helper.rb:96:in `block (2 levels) in <top (required)>'
 # ./vendor/bundle/ruby/2.3.0/gems/rspec-wait-0.0.9/lib/rspec/wait.rb:46:in `block (2 levels) in <top (required)>'

Finished in 27.04 seconds (files took 1.98 seconds to load)
198 examples, 1 failure

Failed examples:

rspec ./test/cmd/readall_spec.rb:2 # brew readall imports all Formulae for a given Tap

I also saw failures in the Travis CI build when I submitted an earlier PR without this change, but they went away when I re-submitted with this addition. I'm not sure what it is about my change in readall that triggers a warning from list.rb. I can investigate if you'd like.

@richiethomas

richiethomas Oct 20, 2017

Contributor

This change was actually prompted by a test failure I saw on my local:

  1) brew readall imports all Formulae for a given Tap
     Failure/Error:
       expect { brew "readall", "--aliases", "--syntax" }
         .to be_a_success
         .and not_to_output.to_stdout
         .and not_to_output.to_stderr
 
      expected #<Proc:0x00000104b03ee8@/usr/local/Homebrew/Library/Homebrew/test/cmd/readall_spec.rb:10> to be a success
 
   ...and:
 
      expected block to not output to stderr, but output "/usr/local/Homebrew/Library/Homebrew/cmd/list.rb:42: warning: `&' interpreted as argument prefix\n"
 # ./test/cmd/readall_spec.rb:10:in `block (2 levels) in <top (required)>'
 # ./test/support/helper/spec/shared_context/integration_test.rb:44:in `block (2 levels) in <top (required)>'
 # ./test/spec_helper.rb:96:in `block (2 levels) in <top (required)>'
 # ./vendor/bundle/ruby/2.3.0/gems/rspec-wait-0.0.9/lib/rspec/wait.rb:46:in `block (2 levels) in <top (required)>'

Finished in 27.04 seconds (files took 1.98 seconds to load)
198 examples, 1 failure

Failed examples:

rspec ./test/cmd/readall_spec.rb:2 # brew readall imports all Formulae for a given Tap

I also saw failures in the Travis CI build when I submitted an earlier PR without this change, but they went away when I re-submitted with this addition. I'm not sure what it is about my change in readall that triggers a warning from list.rb. I can investigate if you'd like.

@MikeMcQuaid MikeMcQuaid merged commit ed28ed7 into Homebrew:master Oct 20, 2017

2 of 3 checks passed

codecov/patch 66.66% of diff hit (target 67.29%)
Details
codecov/project 68.15% (+0.86%) compared to 76cd7c7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Oct 20, 2017

Member

Thanks for your first contribution to Homebrew, @richiethomas! Without people like you submitting PRs we couldn't run this project. You rock!

Member

MikeMcQuaid commented Oct 20, 2017

Thanks for your first contribution to Homebrew, @richiethomas! Without people like you submitting PRs we couldn't run this project. You rock!

@richiethomas richiethomas deleted the richiethomas:refactor_uses branch Oct 23, 2017

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.