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

audit: Port audit_options non-strict rules to rubocop and add tests #2879

Merged
merged 2 commits into from Jul 14, 2017

Conversation

Projects
None yet
3 participants
@GauthamGoli
Copy link
Member

GauthamGoli commented Jul 9, 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?

#569
This PR ports non-strict rules of audit_options method to a custom cop. strict and new-formula rules would be dealt in separate PRs for ease of review.

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

A few tweaks but almost good to go. Thanks!

return node.const_name if node.type == :const
node.const_name
when :sym
node.children[0].to_s

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 10, 2017

Member

I'd use .first here, just reads a little nicer.

module FormulaAudit
# This cop audits `options` in Formulae
class Options < FormulaCop
DEPRICATION_MSG = "macOS has been 64-bit only since 10.6"\

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 10, 2017

Member

DEPRECATION_MSG. I'd probably avoid wrapping this line at 80 characters because it hards readability in this case.

@@ -102,14 +102,11 @@ def find_instance_method_call(node, instance, method_name)

# Returns nil if does not depend on dependency_name
# args: node - dependency_name - dependency's name
def depends_on?(dependency_name)
def depends_on?(dependency_name, *types)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 10, 2017

Member

Don't care this time but in general it's worth putting changes that aren't related to the specific cop into a separate PR. Leave it here for now, though.

This comment has been minimized.

@GauthamGoli

GauthamGoli Jul 10, 2017

Member

Okay will make sure for it to not happen again.
Found it a bit tiresome to commit only few lines of a file. 😅

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_option_rubocop_1 branch from c36f03a to e087952 Jul 10, 2017

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Jul 10, 2017

@MikeMcQuaid Pushed the changes.

@JCount

JCount approved these changes Jul 10, 2017

Copy link
Contributor

JCount left a comment

This looks good. Good work, if @MikeMcQuaid is fine with the changes, it should be ready to ship.

module FormulaAudit
# This cop audits `options` in Formulae
class Options < FormulaCop
DEPRICATION_MSG = "macOS has been 64-bit only since 10.6 so 32-bit options are deprecated.".freeze

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 11, 2017

Member

DEPRICATION_MSG -> DEPRECATION_MSG and add a newline below this line.

end
EOS

DEPRICATION_MSG = "macOS has been 64-bit only since 10.6"\

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 11, 2017

Member

Just in case you do this in other tests too: I'd read this constant from the class itself rather than duplicating it in the test.

This comment has been minimized.

@GauthamGoli

GauthamGoli Jul 12, 2017

Member

Thanks. Made the changes accordingly.

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_option_rubocop_1 branch from e087952 to d296a6b Jul 11, 2017

# This cop audits `options` in Formulae
class Options < FormulaCop
DEPRECATION_MSG = "macOS has been 64-bit only since 10.6 so 32-bit options are deprecated.".freeze
def audit_formula(_node, _class_node, _parent_class_node, body_node)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 12, 2017

Member

Newline above this line.

end
EOS

expected_offenses = [{ message: described_class::DEPRECATION_MSG,

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 12, 2017

Member

👍 Can you check if there's any other tests that could use a similar cleanup?

This comment has been minimized.

@GauthamGoli

GauthamGoli Jul 12, 2017

Member

Yeah. bottle_block_cop and conflicts_cop. Should I push it in this PR?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 12, 2017

Member

Yeh, thanks!

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_option_rubocop_1 branch from d296a6b to 222af82 Jul 13, 2017

@MikeMcQuaid MikeMcQuaid merged commit 20db547 into Homebrew:master Jul 14, 2017

3 checks passed

codecov/patch 85% of diff hit (target 65.65%)
Details
codecov/project 65.67% (+0.02%) compared to bff6af7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 14, 2017

Thanks again @GauthamGoli!

@JCount

This comment has been minimized.

Copy link
Contributor

JCount commented Jul 14, 2017

@GauthamGoli 🎉 good work!

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