Port audit_desc audit rules to a cop #2242

GauthamGoli · 2017-03-01T22:24:50Z

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 is in continuation of the PR #1873 and issue #569
This PR ports audit_desc audit rules into a rubocop custom cop. The rules are

Check if desc exists in a Formula
Check if desc size is > 80
Check if desc begins with an article
Check for proper usage of command-line in desc
Check if desc contains the formula name

The previous tests for the ported audit rules in audit_spec.rb have been removed. ~~I am working on adding tests for the Custom Cops. I have the basic code, but I'm currently trying to figure out where to place the code for the tests to get executed.~~
New tests for the RuboCop custom cops have been added and Gemfile has been updated.

GauthamGoli · 2017-03-02T08:17:37Z

brew style without any args runs style checks on whole Homebrew Library which is why the build is failing as custom cops are running on files outside Library/Taps/homebrew/.

Instead we want Custom Cops to be run only on formulae files. Can I modify style.rb to achieve this? (To exclude custom cop checks on core code and include them on formulae)

Edit: This may not be a good idea, if we plan on adding custom cops in the future for not just brew audit rules

reitermarkus · 2017-03-02T13:08:39Z

rubocop-cask is checking checking if the file is a Cask,

https://github.com/caskroom/rubocop-cask/blob/eca041681dc554996a926173ec1d0c378efaecd3/lib/rubocop/cask/extend/node.rb#L14-L20

and skips it if it isn't.

https://github.com/caskroom/rubocop-cask/blob/cd020379b3a38d4dc20a23eb6c596b5f39414565/lib/rubocop/cop/cask/mixin/cask_help.rb#L9

GauthamGoli · 2017-03-02T13:15:57Z

@reitermarkus I too added a check to see if its a Formula
19f693d#diff-166fafc45d1d65494760d2fb578d6e46R8

But the problem is there are formulae defined outside Library/Taps/homebrew/
https://travis-ci.org/Homebrew/brew/jobs/206899446#L266-L279
and brew style would check those files too, hence the errors. I can may be trivially add desc in those files to get around this?

reitermarkus · 2017-03-02T13:21:06Z

I too added a check for the same

Ok, so there is a on_class method, do you know if there is a on_file method? This way we could simply skip all files under Library/Homebrew.

reitermarkus · 2017-03-02T13:27:15Z

This looks like it could do what we want: https://github.com/bbatsov/rubocop/blob/9754e06f468631e9145b72f30e23e4cc0883bdb8/lib/rubocop/cop/style/file_name.rb#L20-L27

GauthamGoli · 2017-03-02T13:37:06Z

Yes, just found that too. Thanks! :)

reitermarkus · 2017-03-02T16:18:00Z

Library/Homebrew/rubocops/formula_desc_cop.rb

+          formula_name = class_node.const_name
+          return unless parent_class_node && parent_class_node.const_name == "Formula" && body
+          check(node, body, formula_name)
+        end


Can you move this to a helper method/module? We'll probably need this for every Formula cop.

MikeMcQuaid · 2017-03-03T13:41:40Z

Library/Homebrew/rubocops/mixins/on_formula_class.rb

+module RuboCop
+  module Cop
+    # Helper module for checking formula class
+    module FormulaDef


To me it makes more sense for there to be a HomebrewCop base class that other cops inherit from as all will want to use this code.

Yeah, this makes more sense, but I was not able to inherit HomebrewCop due to some error. Found a fix. Will push the changes.

MikeMcQuaid · 2017-03-03T13:42:06Z

Library/Homebrew/rubocops/mixins/on_formula_class.rb

+    # Helper module for checking formula class
+    module FormulaDef
+      def file_inside_formula_root?
+        return false unless processed_source.path =~ %r{#{ENV["HOMEBREW_LIBRARY"]}\/Taps\/homebrew}


Don't think HOMEBREW_LIBRARY will be set when using rubocop outside of brew style?

Should I use %r{\/Taps\/homebrew} without HOMEBREW_LIBRARY ?

Yeh, that sounds like a sensible default. Don't need \/ inside a %r, though!

MikeMcQuaid · 2017-03-03T13:44:25Z

Library/Homebrew/rubocops/formula_desc_cop.rb

+              length = call_node.children[2].source_range.size
+              sourcerange = source_range(source_buffer, line_number, column, length)
+              message = <<-EOS.strip_indent
+                        Description is too long. \"name: desc\" should be less than 80 characters.


Indent this two characters after message and use EOS.undent. Are you sure you need to escape \"?

MikeMcQuaid · 2017-03-03T13:44:46Z

Library/Homebrew/rubocops/formula_desc_cop.rb

+        def check(node, body, formula_name)
+          body.each_child_node(:send) do |call_node|
+            _receiver, call_name, args = *call_node
+            next unless call_name == :desc && !args.children[0].empty?


unless && is generally hard to parse; can you flip this to an if?

MikeMcQuaid · 2017-03-03T13:45:06Z

Library/Homebrew/rubocops/formula_desc_cop.rb

+              add_offense(call_node, sourcerange, message)
+            end
+
+            match_object = description.match(/([Cc]ommand ?line)/)


Could make this regex case-insensitive?

reitermarkus · 2017-03-04T00:16:13Z

Library/Homebrew/rubocops/extend/Homebrewcop.rb

@@ -0,0 +1,19 @@
+module RuboCop
+  module Cop
+    class HomebrewCop < Cop


I'd call this FormulaCop. Also, use snake_case for the filename.

MikeMcQuaid

Lots of comments here. I'm being really pedantic just because I want to try and get these super easy to contribute more but 👍 on all your work here.

MikeMcQuaid · 2017-03-05T10:38:23Z

Library/Homebrew/rubocops/bottle_block_cop.rb

        end

        private

+        def check(body)


Not totally clear to me what body is here; could you explain so we can maybe pick a better name?

body is the RuboCop::AST::Node instance of everything that's inside the formula's class.
The child nodes of body are also instances of RuboCop::AST::Node

How about formula_class_body_node?

MikeMcQuaid · 2017-03-05T10:38:30Z

Library/Homebrew/rubocops/bottle_block_cop.rb

        end

        private

+        def check(body)
+          body.each_child_node(:block) do |block_node|


Not totally clear to me what block_node is here; could you explain so we can maybe pick a better name?

body.each_child_node(:block) calls the given block for each child node which has child.type==:block. (We want to iterate only over blocks and find bottle block).
The given block for body.each_child_node(:block) takes block_node as the argument.

How about possible_bottle_block_node or similar? Feel free to disagree, that's clearer now anyway.

Could also potentially extract to a method that gets block nodes given the method name?

MikeMcQuaid · 2017-03-05T10:38:58Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+
+      def file_inside_formula_root?
+        return false unless processed_source.path =~ %r{/Taps/homebrew}
+        true


!(processed_source.path =~ %r{/Taps/homebrew}).nil? maybe?

Yes, This is more elegant 👍

This will exclude Formulae in all Taps other than homebrew/*.

Actually, this may not be necessary. We can use Include and Exclude options for individual cops in .rubocops.yml to pass file/directory patterns to decide whether cops will run or not. I will update the PR.
@reitermarkus Where else are the Formulae located?

Where else are the Formulae located?

In personal/custom Taps, and I think you can also pass Ruby files directly to brew style.

Yeah you're right. Thanks. I'll think of some way to handle this.

MikeMcQuaid · 2017-03-05T10:39:17Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+        true
+      end
+
+      def on_class(node)


Not sure what node is here; could you explain so we can figure out a better name?

RuboCop processes the source code and creates an Abstract Syntax Tree(AST) representation, and traverses the AST composed of nodes.
As it encounters a node of some type, a message is sent to cop instance class hierarchy by running send(:"on_#{node.type}", node)
and by defining on_class we can process the node and check for violations.

Ok, makes sense as-is thanks.

Maybe worth a comment stating that on_class is called by RuboCop and is the main entry point.

MikeMcQuaid · 2017-03-05T10:39:41Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+      end
+
+      def on_class(node)
+        return unless file_inside_formula_root?


I wonder if this could be done in a shared initializer?

Can you please explain what's a shared initializer and why would we want to use it?

I mean a def initialize method that's shared between all FormulaCops but I change my mind on that now so: ignore me.

MikeMcQuaid · 2017-03-05T10:40:10Z

Library/Homebrew/rubocops/formula_desc_cop.rb

+      # - Checks if `desc` contains the formula name
+
+      class FormulaDesc < FormulaCop
+        def on_formula(node, class_node, _parent_class_node, body)


Not super clear what on_formula means or the various nodes?

Inside FormulaCop there's a method on_class which does the following sequentially

Checks if the file being processed is inside Taps/Homebrew/

If so, check if current class is being inherited from Formula ( Thus making sure that its a Homebrew Formula)

Call on_formula with arguments below. (on_formula is not yet defined as we will define it inside the CustomCop which inherits from FormulaCop)

node is the RuboCop::AST::node instance of whole formula

class_node is the RuboCop::AST::node instance of formula's name

_parent_class_node is the RuboCop::AST::node instance of Formula

body is the RubCop::AST::node for everything thats inside the Formula

class_node, _parent_class_node, body are children of node and may be we can omit passing node as the argument to on_formula as its redundant.

The inheritance and on_formula definition is so that, inside custom cop, we are only concerned with writing rules to check Homebrew Formula coding style violations, while the boilerplate code resides inside FormulaCop.

I'm wondering if it's worth being called audit_formula; the on_ naming scheme is better if people are familiar with RuboCop but I'd think the majority won't be.

MikeMcQuaid · 2017-03-05T10:40:43Z

Library/Homebrew/rubocops/formula_desc_cop.rb

+
+        private
+
+        def check(node, body, formula_name)


Would be good to figure out a way to extract more of the Rubocop/Ruby parse tree logic here from the audit logic.

Perhaps by generally splitting this into a bunch of smaller methods.

While trying to split them into smaller methods, line_number, line_begin_pos, desc_begin_pos, call_node are required as arguments in every method. Can I make them instance variables?

Can you push the changes? Otherwise it's hard to tell if instance variables make sense.

Agreed; go for it.

I've made an additional commit and also some style checks won't be passing. I will squash the commits later once code gets reviewed.

MikeMcQuaid · 2017-03-05T10:41:05Z

Library/Homebrew/rubocops/formula_desc_cop.rb

+              column = desc_begin_pos+match_object.begin(0)-line_begin_pos+1
+              length = match_object.to_s.length
+              sourcerange = source_range(source_buffer, line_number, column, length)
+              add_offense(call_node, sourcerange, "Description should use \"command-line\" instead of \"#{match_object}\"")


A linebreak here would be good to make this a little shorter

MikeMcQuaid · 2017-03-07T16:49:20Z

Library/Homebrew/rubocops/bottle_block_cop.rb

        end

        private

+        def check(body)


How about formula_class_body_node?

MikeMcQuaid · 2017-03-07T16:53:48Z

Library/Homebrew/rubocops/bottle_block_cop.rb

        end

        private

+        def check(body)
+          body.each_child_node(:block) do |block_node|


How about possible_bottle_block_node or similar? Feel free to disagree, that's clearer now anyway.

MikeMcQuaid · 2017-03-07T16:54:19Z

Library/Homebrew/rubocops/bottle_block_cop.rb

+        def check(body)
+          body.each_child_node(:block) do |block_node|
+            next if block_length(block_node).zero?
+            method, _args, block_body = *block_node


I reckon just using _ instead of _args for unused arguments might be easier to follow

If there are multiple such unused variables in the same block, instead of prefixing _ for all of them, using just _ , would it be okay?

Yeh, I think using _ instead of all of them would be good 👍

MikeMcQuaid · 2017-03-07T16:55:20Z

Library/Homebrew/rubocops/bottle_block_cop.rb

        end

        private

+        def check(body)
+          body.each_child_node(:block) do |block_node|


Could also potentially extract to a method that gets block nodes given the method name?

MikeMcQuaid · 2017-03-07T16:55:45Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+        true
+      end
+
+      def on_class(node)


Ok, makes sense as-is thanks.

MikeMcQuaid · 2017-03-07T16:56:46Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+      end
+
+      def on_class(node)
+        return unless file_inside_formula_root?


I mean a def initialize method that's shared between all FormulaCops but I change my mind on that now so: ignore me.

MikeMcQuaid · 2017-03-07T16:57:01Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+      def on_class(node)
+        return unless file_inside_formula_root?
+        class_node, parent_class_node, body = *node
+        return unless parent_class_node && parent_class_node.const_name == "Formula"


Use if as unless && is hard to follow.

This line could also perhaps be split into a dedicated method.

MikeMcQuaid · 2017-03-07T16:59:24Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+        true
+      end
+
+      def on_class(node)


Maybe worth a comment stating that on_class is called by RuboCop and is the main entry point.

MikeMcQuaid · 2017-03-07T17:19:45Z

Library/Homebrew/rubocops/formula_desc_cop.rb

+      # - Checks if `desc` contains the formula name
+
+      class FormulaDesc < FormulaCop
+        def on_formula(node, class_node, _parent_class_node, body)


I'm wondering if it's worth being called audit_formula; the on_ naming scheme is better if people are familiar with RuboCop but I'd think the majority won't be.

MikeMcQuaid · 2017-03-07T17:20:04Z

Library/Homebrew/rubocops/formula_desc_cop.rb

+
+        private
+
+        def check(node, body, formula_name)


Perhaps by generally splitting this into a bunch of smaller methods.

MikeMcQuaid · 2017-03-12T09:17:01Z

Library/.rubocop.yml

@@ -8,7 +8,17 @@ AllCops:

 require: ./Homebrew/rubocops.rb

+Homebrew/FormulaCop:


Maybe worth putting this in another directory so it doesn't need to be disabled like this?

MikeMcQuaid · 2017-03-12T09:17:34Z

Library/.rubocop.yml

 Homebrew/CorrectBottleBlock:
+  Include:
+    - '**/Taps/homebrew/**/*.rb'


If this will need to be repeated for every cop I think the existing approach was superior.

Also, as I said before, this will exclude every Formula in personal/custom Taps and Ruby files that are passed directly to brew style.

We need to exclude HOMEBREW_LIBRARY_PATH rather than include Taps/**.

I think just using '**/Taps/**/*.rb' may be sufficient? We don't want these applying to non-HOMEBREW_LIBRARY_PATH Ruby files.

They won't apply to all Ruby files, they will only match files with class ***** < Formula anyways.

MikeMcQuaid · 2017-03-12T09:19:04Z

Library/Homebrew/rubocops/formula_desc_cop.rb

+            @line_number = call_node.loc.line
+            @line_begin_pos = call_node.source_range.source_buffer.line_range(call_node.loc.line).begin_pos
+            @desc_begin_pos = call_node.children[2].source_range.begin_pos
+            @call_node = call_node


I'm thinking some of this could be extracted into a method named find_method_by_name(:desc) which then returns the node or perhaps just the parameters? That's the type of thing I'm thinking with splitting this into smaller methods; have generic functions like find_method_by_name and later when needed find_block_by_name etc. which could take method name and optional block_name arguments and return e.g. the parameters or information as a hash. This should all live in FormulaCop so that it's not necessary to really understand RuboCop to implement a FormulaCop subclass and you can just call these documented, public methods. What do you think?

Yes, this makes sense. Wrapping rubocop specific code into documented methods would make it easier for other maintainers to add custom cops. But, I think, (as of now) its not possible to make writing custom cops to be completely rubocop agnostic as every possible requirement cannot be known beforehand, although we can try to cover most of the common cases,
And I think closures would be a good fit in this use case. I am pushing the code, please let me know further comments.
I apologize for the delay, as I was on a vacation.

reitermarkus · 2017-03-16T18:24:32Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+          # This method is called by RuboCop and is the main entry point
+          class_node, parent_class_node, body = *node
+          return unless a_formula_class?(parent_class_node)
+          audit_formula(node, class_node, parent_class_node, body)


Add a return unless respond_to?(:audit_formula) before this, so you can remove Homebrew/FormulaCop from the RuboCop config.

Thanks! Added it.

reitermarkus · 2017-03-16T18:28:34Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+  module Cop
+    module Homebrew
+      class FormulaCop < Cop
+        @registry = Cop.registry


What is this used for?

Ah! That's a hack. I haven't found a solution for it yet. Without this line, there's an error undefined method `enlist' for nil:NilClass being thrown. After further debugging, found that @registry = Cop.registry line inside Cop is not getting executed when FormulaCop is inheriting from Cop

reitermarkus · 2017-03-16T18:30:11Z

Library/Homebrew/rubocops/formula_desc_cop.rb

+              offense_source_range = source_range(source_buffer, line_number, column, length)
+              desc_length_offense_msg = <<-EOS.undent
+              Description is too long. "name: desc" should be less than #{max_desc_length} characters.
+              Length is calculated as #{formula_name} + desc. (currently #{"#{formula_name}: #{description}".length})


Indent these lines by 2 spaces.

MikeMcQuaid · 2017-03-17T14:22:20Z

Library/.rubocop.yml

 Homebrew/CorrectBottleBlock:
+  Include:
+    - '**/Taps/homebrew/**/*.rb'


Let's remove these includes and leave the logic in the cops themselves unless there's a way to do this that avoids duplicating this line for every cop.

As @reitermarkus pointed out, due to Include in the .rubocop.yml the cops won't be run on personal/custom Taps and the files directly passed as arguments which do not match the pattern under Include.
To resolve this issue I need some clarification. brew style command, runs on all the files inside HOMEBREW_LIBRARY_PATH while brew audit runs only on the forumlae files.
Do we want(or is it okay for) the custom cops to get executed under brew audit and brew style also?

As @reitermarkus pointed out, due to Include in the .rubocop.yml the cops won't be run on personal/custom Taps and the files directly passed as arguments which do not match the pattern under Include.

I think it's OK for the taps to run on unnecessary files as they are still a pretty quick no-op.

Do we want(or is it okay for) the custom cops to get executed under brew audit and brew style also?

Yeh, they should be executed by all of them I think (including rubocop obviously, just to be explicit)

There are classes in files
compat/formula_specialties.rb
test/exceptions_spec.rb
test/support/fixtures/testball.rb
test/support/fixtures/testball_bottle.rb
inheriting from Formula and are returning cop violations as they don't have formula desc, causing the build to fail. If desc can be added in the above files, then we don't have to repeat Include in .rubocop.yml

If desc can be added in the above files, then we don't have to repeat Include in .rubocop.yml

Why not just check if the file path contains /Library/Homebrew/ and exclude those? Didn't we have this approach already before?

Well, I was trying to keep the cop code and config code separate in their respective files. But turns out, keeping the Include logic in FormulaCop is more elegant. Thanks @reitermarkus and @MikeMcQuaid, now Personal Taps can be passed as arguments.
I added the logic in Formula Cop and pushed the new code. I also used rebase to add another test in bottle_block_cop_spec in a past commit to pass coverage test.

MikeMcQuaid · 2017-03-17T14:25:28Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+          audit_formula(node, class_node, parent_class_node, body)
+        end
+
+        def find_method(node, method_name)


def find_node_method_by_name(node, method_name) maybe?

MikeMcQuaid · 2017-03-17T14:33:43Z

Library/Homebrew/rubocops/bottle_block_cop.rb

-          return unless method_name == :bottle
-          check_revision?(body)
+        def audit_formula(_node, _class_node, _parent_class_node, formula_class_body_node)
+          check(formula_class_body_node)


Ideally this method would contain code like the following:

def audit_formula bottle = find_block(:bottle) return unless bottle if method_called?(bottle, :revision) problem "Use rebuild instead of revision in a bottle block" end end

The find_block and method_called? methods should be defined in formula_cop.rb and the various instance variables set by on_class or something it calls.

MikeMcQuaid · 2017-03-17T14:34:54Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+          end
+        end
+
+        def node_line_begin_pos(node)


def line_start_column(node) maybe?

MikeMcQuaid · 2017-03-17T14:35:12Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+          node.source_range.source_buffer.line_range(node.loc.line).begin_pos
+        end
+
+        def node_begin_pos(node)


def start_column(node) maybe?

MikeMcQuaid · 2017-03-17T14:36:12Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+          node.const_name
+        end
+
+        def node_size(node)


def size(node) maybe?

MikeMcQuaid · 2017-03-17T14:36:46Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+          node.source_range.size
+        end
+
+        def get_source_buffer(node)


source_buffer(node) maybe?

MikeMcQuaid · 2017-03-17T14:36:57Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+          node.source_range.source_buffer
+        end
+
+        def get_str_content(node)


string_content(node) maybe?

MikeMcQuaid · 2017-03-17T14:37:11Z

Library/Homebrew/rubocops/extend/formula_cop.rb

+
+        private
+
+        def a_formula_class?(parent_class_node)


is_formula_class?(node) maybe?

Without the is_, I'd say.

MikeMcQuaid · 2017-03-17T14:45:40Z

Library/Homebrew/rubocops/formula_desc_cop.rb

+      # - Checks for correct usage of `command-line` in `desc`
+      # - Checks if `desc` contains the formula name
+      class FormulaDesc < FormulaCop
+        def audit_formula(node, class_node, _parent_class_node, body)


Ideally this method would contain code like the following:

def audit_formula desc_call = find_method_call(:desc) return unless desc_call desc = parameters(desc_call).first max_length = 80 if desc.length > max_length problem <<-EOS.undent Description is too long. "name: desc" should be less than #{max_length} characters. Length is calculated as #{formula_name} + desc. (currently #{"#{formula_name}: #{desc}".length}) EOS if desc =~ /(command ?line)/i problem "Description should use \"command-line\" instead of \"#{$1}\"" end ... end

The find_method_call and parameters? methods should be defined in formula_cop.rb and the various instance variables set by on_class or something it calls and by e.g. find_method_call and parameters so problem can access e.g. the source_range.

Obviously this can (and will) vary but just giving you a heads up for the level of complexity I think would be ideal for these cops to hide the implementation details.

Thanks for the above sample code. 👍 Now its clear which way to implement.
desc_call = find_method_call(:desc) we are assuming that there would be only one method call. There might be multiple calls to the same method. Should it return an array instead?

I think having find_method_call which always returns the first would be a sensible first step. If it's needed later we can have find_method_calls for e.g. checking use of things like inreplace and then loop over them.

While trying to implement the above I faced an issue. Just by having

if desc =~ /(command ?line)/i problem "Description should use \"command-line\" instead of \"#{$1}\"" end

its difficult to set all the instance variables required for pinpointing the source_range of the style violation. So I added a method check_regex_match. Now the code looks like

match_object = check_regex_match(desc, /(command ?line)/i) if match_object problem "Description should use \"command-line\" instead of \"#{match_object}\"" end

How about:

if match = regex_match_group(desc, /(command ?line)/i) problem "Description should use \"command-line\" instead of \"#{match_object}\"" end

MikeMcQuaid · 2017-03-30T19:38:23Z

This is looking pretty good to merge for me now. @reitermarkus, any thoughts?

Comments addressed

MikeMcQuaid · 2017-04-05T07:41:37Z

Great work again @GauthamGoli! My suggestion for the next rules would be https://github.com/Homebrew/brew/blob/master/Library/Homebrew/dev-cmd/audit.rb#L251-L309; these are the ones I personally tend to wish to have in my editor the most.

Port audit_desc rules to cop

19f693d

GauthamGoli force-pushed the audit_custom_cops branch from dbfd76f to 19f693d Compare March 2, 2017 07:21

reitermarkus previously requested changes Mar 2, 2017

View reviewed changes

GauthamGoli force-pushed the audit_custom_cops branch from 3e1fbd8 to fa79ca9 Compare March 3, 2017 12:07

MikeMcQuaid requested changes Mar 3, 2017

View reviewed changes

GauthamGoli force-pushed the audit_custom_cops branch 3 times, most recently from 02bbad4 to 93f719a Compare March 3, 2017 21:57

reitermarkus reviewed Mar 4, 2017

View reviewed changes

GauthamGoli force-pushed the audit_custom_cops branch 2 times, most recently from cae3856 to 5687619 Compare March 4, 2017 08:56

MikeMcQuaid requested changes Mar 5, 2017

View reviewed changes

MikeMcQuaid requested changes Mar 7, 2017

View reviewed changes

Create FormulaCop base class to reuse helper functions in custom cops

d32978b

GauthamGoli force-pushed the audit_custom_cops branch 2 times, most recently from 72b0b0c to d54504b Compare March 8, 2017 08:22

MikeMcQuaid reviewed Mar 12, 2017

View reviewed changes

reitermarkus reviewed Mar 16, 2017

View reviewed changes

GauthamGoli force-pushed the audit_custom_cops branch from 4b1b8ab to 7e43bb9 Compare March 16, 2017 18:40

MikeMcQuaid requested changes Mar 17, 2017

View reviewed changes

GauthamGoli force-pushed the audit_custom_cops branch 2 times, most recently from 01296f7 to 2feded9 Compare March 19, 2017 11:13

GauthamGoli force-pushed the audit_custom_cops branch 2 times, most recently from 1e657e3 to 9c93a2a Compare March 26, 2017 08:02

GauthamGoli added 3 commits March 26, 2017 15:26

Add RSpec tests for bottle_block and formula_desc cops

4e57f42

Encapsulate formula desc offense checking logic into methods

febc108

Wrap rubocop specific code into methods inside FormulaCop

a693ca3

GauthamGoli force-pushed the audit_custom_cops branch from 9c93a2a to a693ca3 Compare March 26, 2017 09:59

MikeMcQuaid approved these changes Mar 30, 2017

View reviewed changes

MikeMcQuaid merged commit 3f51406 into Homebrew:master Apr 5, 2017

GauthamGoli mentioned this pull request Apr 8, 2017

audit: audit_components method to rubocops and tests #2465

Merged

5 tasks

GauthamGoli deleted the audit_custom_cops branch April 27, 2017 16:49

GauthamGoli restored the audit_custom_cops branch April 27, 2017 16:49

Homebrew locked and limited conversation to collaborators May 3, 2018

		@@ -8,7 +8,17 @@ AllCops:

		require: ./Homebrew/rubocops.rb

		Homebrew/FormulaCop:

Port audit_desc audit rules to a cop #2242

Port audit_desc audit rules to a cop #2242

Conversation

GauthamGoli commented Mar 1, 2017 • edited Loading

GauthamGoli commented Mar 2, 2017 • edited Loading

reitermarkus commented Mar 2, 2017

GauthamGoli commented Mar 2, 2017 • edited Loading

reitermarkus commented Mar 2, 2017

reitermarkus commented Mar 2, 2017

GauthamGoli commented Mar 2, 2017 • edited Loading

reitermarkus Mar 2, 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

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 left a 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

GauthamGoli Mar 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GauthamGoli Mar 5, 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

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

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

reitermarkus Mar 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reitermarkus Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GauthamGoli commented Mar 1, 2017 •

edited

Loading

GauthamGoli commented Mar 2, 2017 •

edited

Loading

GauthamGoli commented Mar 2, 2017 •

edited

Loading

GauthamGoli commented Mar 2, 2017 •

edited

Loading

reitermarkus Mar 2, 2017 •

edited

Loading

GauthamGoli Mar 5, 2017 •

edited

Loading

GauthamGoli Mar 5, 2017 •

edited

Loading

reitermarkus Mar 12, 2017 •

edited

Loading

reitermarkus Mar 16, 2017 •

edited

Loading

GauthamGoli Mar 16, 2017 •

edited

Loading