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

Port audit_desc audit rules to a cop #2242

Merged
merged 5 commits into from Apr 5, 2017

Conversation

Projects
None yet
3 participants
@GauthamGoli
Copy link
Member

GauthamGoli commented Mar 1, 2017

  • 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 GauthamGoli force-pushed the GauthamGoli:audit_custom_cops branch from dbfd76f to 19f693d Mar 2, 2017

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Mar 2, 2017

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

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Mar 2, 2017

@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

This comment has been minimized.

Copy link
Member

reitermarkus commented Mar 2, 2017

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

This comment has been minimized.

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Mar 2, 2017

Yes, just found that too. Thanks! :)

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

This comment has been minimized.

@reitermarkus

reitermarkus Mar 2, 2017

Member

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

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_custom_cops branch from 3e1fbd8 to fa79ca9 Mar 3, 2017

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 3, 2017

Member

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.

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 3, 2017

Member

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

# 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}

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 3, 2017

Member

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

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 3, 2017

Member

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 4, 2017

Member

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

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.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 3, 2017

Member

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

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?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 3, 2017

Member

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

add_offense(call_node, sourcerange, message)
end

match_object = description.match(/([Cc]ommand ?line)/)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 3, 2017

Member

Could make this regex case-insensitive?

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_custom_cops branch 3 times, most recently from 02bbad4 to 93f719a Mar 3, 2017

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

This comment has been minimized.

@reitermarkus

reitermarkus Mar 4, 2017

Member

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

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 4, 2017

Member

Thanks.

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_custom_cops branch 2 times, most recently from cae3856 to 5687619 Mar 4, 2017

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

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.

end

private

def check(body)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 5, 2017

Member

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

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 5, 2017

Member

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

How about formula_class_body_node?

end

private

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 5, 2017

Member

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

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 5, 2017

Member

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.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

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


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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 5, 2017

Member

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

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 5, 2017

Member

Yes, This is more elegant 👍

This comment has been minimized.

@reitermarkus

reitermarkus Mar 7, 2017

Member

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

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 7, 2017

Member

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?

This comment has been minimized.

@reitermarkus

reitermarkus Mar 8, 2017

Member

Where else are the Formulae located?

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

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 8, 2017

Member

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

true
end

def on_class(node)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 5, 2017

Member

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

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 5, 2017

Member

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.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

Ok, makes sense as-is thanks.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

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

end

def on_class(node)
return unless file_inside_formula_root?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 5, 2017

Member

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

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 7, 2017

Member

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

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

# - Checks if `desc` contains the formula name

class FormulaDesc < FormulaCop
def on_formula(node, class_node, _parent_class_node, body)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 5, 2017

Member

Not super clear what on_formula means or the various nodes?

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 5, 2017

Member

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.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

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.


private

def check(node, body, formula_name)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 5, 2017

Member

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

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

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 10, 2017

Member

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?

This comment has been minimized.

@reitermarkus

reitermarkus Mar 11, 2017

Member

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 11, 2017

Member

Agreed; go for it.

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 11, 2017

Member

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.

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}\"")

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 5, 2017

Member

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

end

private

def check(body)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

How about formula_class_body_node?

end

private

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

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

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

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

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 8, 2017

Member

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 8, 2017

Member

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

end

private

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

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

true
end

def on_class(node)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

Ok, makes sense as-is thanks.

end

def on_class(node)
return unless file_inside_formula_root?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

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

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"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

Use if as unless && is hard to follow.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

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

true
end

def on_class(node)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

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

# - Checks if `desc` contains the formula name

class FormulaDesc < FormulaCop
def on_formula(node, class_node, _parent_class_node, body)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

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.


private

def check(node, body, formula_name)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 7, 2017

Member

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

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_custom_cops branch 2 times, most recently from 72b0b0c to d54504b Mar 8, 2017

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

require: ./Homebrew/rubocops.rb

Homebrew/FormulaCop:

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 12, 2017

Member

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

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 12, 2017

Member

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

This comment has been minimized.

@reitermarkus

reitermarkus Mar 12, 2017

Member

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/**.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 12, 2017

Member

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

This comment has been minimized.

@reitermarkus

reitermarkus Mar 12, 2017

Member

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

@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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 12, 2017

Member

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?

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 16, 2017

Member

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.

# 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)

This comment has been minimized.

@reitermarkus

reitermarkus Mar 16, 2017

Member

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

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 16, 2017

Member

Thanks! Added it.

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

This comment has been minimized.

@reitermarkus

reitermarkus Mar 16, 2017

Member

What is this used for?

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 16, 2017

Member

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

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})

This comment has been minimized.

@reitermarkus

reitermarkus Mar 16, 2017

Member

Indent these lines by 2 spaces.

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_custom_cops branch from 4b1b8ab to 7e43bb9 Mar 16, 2017

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 17, 2017

Member

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.

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 25, 2017

Member

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?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 25, 2017

Member

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)

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 25, 2017

Member

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

This comment has been minimized.

@reitermarkus

reitermarkus Mar 25, 2017

Member

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?

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 26, 2017

Member

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.

audit_formula(node, class_node, parent_class_node, body)
end

def find_method(node, method_name)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 17, 2017

Member

def find_node_method_by_name(node, method_name) maybe?

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)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 17, 2017

Member

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.

end
end

def node_line_begin_pos(node)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 17, 2017

Member

def line_start_column(node) maybe?

node.source_range.source_buffer.line_range(node.loc.line).begin_pos
end

def node_begin_pos(node)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 17, 2017

Member

def start_column(node) maybe?

node.const_name
end

def node_size(node)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 17, 2017

Member

def size(node) maybe?

node.source_range.size
end

def get_source_buffer(node)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 17, 2017

Member

source_buffer(node) maybe?

node.source_range.source_buffer
end

def get_str_content(node)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 17, 2017

Member

string_content(node) maybe?


private

def a_formula_class?(parent_class_node)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 17, 2017

Member

is_formula_class?(node) maybe?

This comment has been minimized.

@reitermarkus

reitermarkus Mar 17, 2017

Member

Without the is_, I'd say.

# - 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)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 17, 2017

Member

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.

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 18, 2017

Member

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?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 18, 2017

Member

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.

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 19, 2017

Member

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 19, 2017

Member

How about:

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

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_custom_cops branch 2 times, most recently from 01296f7 to 2feded9 Mar 18, 2017

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_custom_cops branch 2 times, most recently from 1e657e3 to 9c93a2a Mar 26, 2017

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_custom_cops branch from 9c93a2a to a693ca3 Mar 26, 2017

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Mar 30, 2017

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

Comments addressed

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

3 checks passed

codecov/patch 76.81% of diff hit (target 60.34%)
Details
codecov/project Absolute coverage decreased by -0.03% but relative coverage increased by +16.46% compared to 422afa0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 5, 2017

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.

@GauthamGoli GauthamGoli referenced this pull request Apr 8, 2017

Merged

audit: audit_components method to rubocops and tests #2465

5 of 5 tasks complete

@GauthamGoli GauthamGoli deleted the GauthamGoli:audit_custom_cops branch Apr 27, 2017

@GauthamGoli GauthamGoli restored the GauthamGoli:audit_custom_cops branch Apr 27, 2017

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

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