Custom Cops for `brew audit` #1873

GauthamGoli · 2017-01-18T10:40:28Z

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.
Have you successfully run brew tests with your changes locally?

This PR implements Custom Cops for a simple rule in brew audit command as discussed in #569

revision in a bottle do block should be renamed to rebuild and Rubocop should autocorrect it to rebuild

More rules can be added/migrated as cops, and RuboCop can be leveraged to check for style violations instead of regular expressions as being done currently when brew audit is ran.

Using --strict would check for CustomCop/style violations.
The newly added --fix option would automatically correct the source code if any CustomCop/style violations are found.

GauthamGoli · 2017-01-18T13:07:22Z

Why did the build fail? brew tests on my local machine shows 0 errors.

MikeMcQuaid · 2017-01-18T14:37:51Z

brew man needs run and brew style failed: https://travis-ci.org/Homebrew/brew/builds/192996433

GauthamGoli · 2017-01-19T16:42:01Z

@MikeMcQuaid Now only brew tests --generic is failing in CI. Strangely , it doesn't fail and shows 0 errors when I run the same command locally.

alyssais · 2017-01-19T16:47:49Z

@GauthamGoli don't worry about it. There's a test suite bug that causes that test to intermittently fail. #1879 fixes it.

MikeMcQuaid · 2017-01-19T17:07:49Z

It's merged so rerunning these tests.

GauthamGoli · 2017-01-20T19:40:38Z

@MikeMcQuaid What should I do for codecov check failures?

MikeMcQuaid · 2017-01-23T08:03:46Z

@GauthamGoli You can ignore them for now 👍

MikeMcQuaid · 2017-01-23T08:04:33Z

Library/Homebrew/rubocops/bottle_block_cop.rb

+          method, _args, body = *node
+          _keyword, method_name = *method
+
+          return unless method_name.equal?(:bottle) && revision?(body)


Make this multiple returns.

MikeMcQuaid · 2017-01-23T08:04:52Z

Library/Homebrew/rubocops/bottle_block_cop.rb

+          lambda do |corrector|
+            # Check for revision
+            _method, _args, body = *node
+            if revision?(body)


next unless revision?(body)

MikeMcQuaid · 2017-01-23T08:05:12Z

Library/Homebrew/rubocops/bottle_block_cop.rb

+        def revision?(body)
+          body.children.each do |method_call_node|
+            _receiver, method_name, _args = *method_call_node
+            if method_name == :revision


return true if method_name == :revision

MikeMcQuaid · 2017-01-23T08:05:45Z

Library/Homebrew/rubocops/bottle_block_cop.rb

+          new_source = ""
+          node.source.each_line do |line|
+            if line =~ /\A\s*revision/
+              line = line.sub("revision", "rebuild")


line.sub!("revision", "rebuild")

MikeMcQuaid · 2017-01-23T08:06:08Z

Library/Homebrew/rubocops/bottle_block_cop.rb

+        def replace_revision(corrector, node)
+          new_source = ""
+          node.source.each_line do |line|
+            if line =~ /\A\s*revision/


Maybe next unless line =~ /\A\s*revision/?

next cannot be used I think because on line 42 , the current line is being appended to new_source

Yeh, sorry, 👍

alyssais · 2017-01-23T19:04:31Z

Library/Homebrew/rubocops/bottle_block_cop.rb

-            new_source << line
+              line.sub!("revision", "rebuild")
+	    end
+	    new_source << line


Looks like something weird's happened to your indentation here.

Ah! Mix up of spaces and tabs due to opening in different editors. Fixing it.

MikeMcQuaid · 2017-01-23T21:09:24Z

Tested locally. The user experience here is amazingly good.

@reitermarkus @jawshooah you may want to consider this approach instead of a separate gem; if they have editor Rubocop integration with this one they'll just start seeing these warnings immediately with zero configuration.

alyssais · 2017-01-23T21:30:56Z

@MikeMcQuaid you just inspired me to set up Rubocop in my editor. This is amazing!

Nice work @GauthamGoli! 👏

alyssais · 2017-01-23T21:36:42Z

One thing: when I test this with a bottle block, my editor highlights bottle rather than rebuild, which doesn't seem quite right. Is that just a problem with my editor's RuboCop integration, or is it fixable?

alyssais · 2017-01-23T21:38:41Z

Library/Homebrew/rubocops/bottle_block_cop.rb

+          method, _args, body = *node
+          _keyword, method_name = *method
+
+          return unless method_name.equal?(:bottle)


I think we should prefer #== over #equal?.

Thanks. Pushed the updated code.

GauthamGoli · 2017-01-24T01:03:09Z

@alyssais Are you using Emacs?

dunn · 2017-01-24T02:03:31Z

Library/Homebrew/rubocops/bottle_block_cop.rb

+        end
+
+        def revision?(body)
+          body.children.each do |method_call_node|


Can this be body.children.any? instead?

Thanks. Pushed the updated code.

alyssais · 2017-01-24T09:51:38Z

@GauthamGoli I'm using Sublime Text 3 with SublimeLinter-rubocop.

GauthamGoli · 2017-01-24T19:12:25Z

@alyssais Able to reproduce this. I think its because, I used on_block method in the custom cop to detect usage of revision inside a bottle do block. As of now, I don't know how to make it highlight revision as the documentation of rubocop on writing custom cops is not so clear for me.

alyssais · 2017-01-24T19:23:40Z

@GauthamGoli fair enough. Let's forget about it for now, then, it's not a big deal. :)

GauthamGoli · 2017-01-25T04:43:00Z

@alyssais I found a fix for this. Will update the code and push it in sometime!

GauthamGoli · 2017-01-25T13:16:04Z

@alyssais Now the linter should highlight revision
@MikeMcQuaid @dunn The code has been simplified. Please review it.

MikeMcQuaid · 2017-01-25T13:18:14Z

Library/Homebrew/rubocops/bottle_block_cop.rb

+        def check_revision?(body)
+          body.children.each do |method_call_node|
+            _receiver, method_name, _args = *method_call_node
+            if method_name == :revision


Could do next unless method_name == :revision to avoid indentation and end

MikeMcQuaid · 2017-01-25T13:19:25Z

@MikeMcQuaid @dunn The code has been simplified. Please review it.

Looks much better/simpler, thanks!

reitermarkus · 2017-02-05T22:42:04Z

docs/brew.1.html

 run before submitting a new formula.</p>

 <p>If no <var>formulae</var> are provided, all of them are checked.</p>

 <p>If <code>--strict</code> is passed, additional checks are run, including RuboCop
-style checks.</p>
+style checks and custom cop checks.</p>


This is a bit redundant, since custom checks are still RuboCop style checks.

Yeah, you're right. Fixed it.

MikeMcQuaid · 2017-02-10T15:21:17Z

Library/.rubocop.yml

@@ -6,6 +6,12 @@ AllCops:
    - '**/Casks/**/*'
    - '**/vendor/**/*'

+require:
+  - ./Homebrew/rubocops/bottle_block_cop.rb


Let's maybe require a since Homebrew/rubocops.rb which in turn includes all our custom cops (just this one for now but we'll add more).

Also could just do on a single line like require: ./Homebrew/rubocops.rb perhaps?

Yes, I was thinking of creating another another .yml file(which will have all custom cops) andrequire it inside .rubocop.yml as the custom cops grow in number. Your idea of require on rubocops.rb is seems much better, but then we'd have to place all the cops in a single huge rubocops.rb rather than having individual files. If thats not a problem, I'll go ahead and change it.

but then we'd have to place all the cops in a single huge rubocops.rb

Not really, we just require all of the individual cops inside rubocops/ with this file, so we don't have to do that inside the YAML file.

Okay, cool. On my way to make the above changes.

Facing an error when implementing the above: in `<module:Cop>':superclass must be a Class (Module given) (TypeError) , trying to find a work around.

@GauthamGoli feel free to post your code if you need a hand working it out! :)

@alyssais I created Library/Homebrew/rubocops.rb, and it has a single line Dir[File.dirname(__FILE__) + "/rubocops/*.rb"].each {|file| require file }

Line 10 in Library/.rubocop.yml is now - ./Homebrew/rubocops.rb

Now just run brew audit --strict <someformula> to see the error.
Further debugging shows the error is thrown at line 72 in /Homebrew/cmd/style.rb
and I think the issue is being unable to require bottle_block_cop.rb

@GauthamGoli it's failing because of this error:

Error: obsolete parameter IndentWhenRelativeTo (for Style/CaseIndentation) found in /Users/alyssa/code/brew/Library/.rubocop.yml `IndentWhenRelativeTo` has been renamed to `EnforcedStyle` obsolete parameter AlignWith (for Lint/EndAlignment) found in /Users/alyssa/code/brew/Library/.rubocop.yml `AlignWith` has been renamed to `EnforcedStyleAlignWith`

Let's just hard-code all the requires for now; dynamic requiring is a bit overkill for now.

MikeMcQuaid · 2017-02-10T15:22:01Z

Library/Homebrew/rubocops/bottle_block_cop.rb

@@ -0,0 +1,36 @@
+module RuboCop
+  module Cop
+    module CustomCops


Is it possible to call this module Homebrew?

module Homebrew has been used in Library/Homebrew/cmd/ and Library/Hombrew/dev-cmd/ , so I thought calling it as Homebrew might cause some sort of confusion/clash ( I am not sure) and named it as CustomCops

@GauthamGoli, this isn't really a problem as it's in another namespace. The only thing to look out for would be to reference the top-level module Homebrew with ::Homebrew inside Rubocop::Cop, but I think we don't even need it in this context.

this isn't really a problem as it's in another namespace

Agreed here.

MikeMcQuaid · 2017-02-12T13:09:34Z

Great work here @GauthamGoli 👏!

Add custom cop to refactor revision to rebuild in bottle block

483c40f

GauthamGoli force-pushed the audit_custom_cops branch from 777988c to b18ffcf Compare January 18, 2017 17:04

Add --fix option to brew audit command

0b3d903

MikeMcQuaid requested changes Jan 23, 2017

View reviewed changes

GauthamGoli force-pushed the audit_custom_cops branch from b18ffcf to 7b6b2e7 Compare January 23, 2017 18:52

alyssais reviewed Jan 23, 2017

View reviewed changes

GauthamGoli force-pushed the audit_custom_cops branch from 7b6b2e7 to 7b68491 Compare January 23, 2017 19:13

MikeMcQuaid approved these changes Jan 23, 2017

View reviewed changes

alyssais suggested changes Jan 23, 2017

View reviewed changes

GauthamGoli force-pushed the audit_custom_cops branch from 7b68491 to d009e97 Compare January 24, 2017 01:01

dunn reviewed Jan 24, 2017

View reviewed changes

GauthamGoli force-pushed the audit_custom_cops branch from d009e97 to 69bb6bb Compare January 24, 2017 05:06

alyssais approved these changes Jan 24, 2017

View reviewed changes

GauthamGoli force-pushed the audit_custom_cops branch 2 times, most recently from bf74c61 to 4321b6b Compare January 25, 2017 05:08

GauthamGoli force-pushed the audit_custom_cops branch from 4321b6b to 9f83f91 Compare January 25, 2017 08:21

MikeMcQuaid reviewed Jan 25, 2017

View reviewed changes

GauthamGoli force-pushed the audit_custom_cops branch from 9f83f91 to 89d5a1f Compare January 25, 2017 14:10

reitermarkus reviewed Feb 5, 2017

View reviewed changes

GauthamGoli force-pushed the audit_custom_cops branch from 89d5a1f to 1f5cf4f Compare February 6, 2017 07:12

Update docs and manpages to include --fix option

1f5cf4f

MikeMcQuaid requested changes Feb 10, 2017

View reviewed changes

GauthamGoli force-pushed the audit_custom_cops branch from 3f225b7 to 8df4a0a Compare February 12, 2017 06:04

Update rubocop config

5dc358c

GauthamGoli force-pushed the audit_custom_cops branch from 8df4a0a to 5dc358c Compare February 12, 2017 09:24

MikeMcQuaid approved these changes Feb 12, 2017

View reviewed changes

MikeMcQuaid merged commit 674e5f1 into Homebrew:master Feb 12, 2017

GauthamGoli mentioned this pull request Mar 1, 2017

Port audit_desc audit rules to a cop #2242

Merged

5 tasks

Homebrew locked and limited conversation to collaborators May 3, 2018

Custom Cops for brew audit #1873

Custom Cops for brew audit #1873

Conversation

GauthamGoli commented Jan 18, 2017 • edited Loading

GauthamGoli commented Jan 18, 2017

MikeMcQuaid commented Jan 18, 2017

GauthamGoli commented Jan 19, 2017

alyssais commented Jan 19, 2017

MikeMcQuaid commented Jan 19, 2017

GauthamGoli commented Jan 20, 2017

MikeMcQuaid commented Jan 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GauthamGoli Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Jan 23, 2017

alyssais commented Jan 23, 2017

alyssais commented Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GauthamGoli commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssais commented Jan 24, 2017

GauthamGoli commented Jan 24, 2017

alyssais commented Jan 24, 2017

GauthamGoli commented Jan 25, 2017

GauthamGoli commented Jan 25, 2017

Choose a reason for hiding this comment

MikeMcQuaid commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Feb 12, 2017

Custom Cops for `brew audit` #1873

Custom Cops for `brew audit` #1873

GauthamGoli commented Jan 18, 2017 •

edited

Loading

GauthamGoli Jan 23, 2017 •

edited

Loading

alyssais commented Jan 23, 2017 •

edited

Loading