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_class to rubocop, add tests and autocorrect #2982

Merged
merged 2 commits into from Sep 5, 2017

Conversation

Projects
None yet
3 participants
@GauthamGoli
Copy link
Member

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

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_class_rubocop_port branch from cca388d to 2b8b78c Jul 30, 2017


def audit_formula(_node, _class_node, parent_class_node, _body_node)
parent_class = class_name(parent_class_node)
return unless DEPRECATED_CLASSES.index(parent_class)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 1, 2017

Member

why .index and not .include? here?

This comment has been minimized.

@GauthamGoli

GauthamGoli Aug 1, 2017

Author Member

Right! Sorry.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 1, 2017

Member

No worries, just checking!

AmazonWebServicesFormula
]

class_node && class_names.index(string_content(class_node))

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 1, 2017

Member

Not sure I understand this change, can you elaborate?

This comment has been minimized.

@GauthamGoli

GauthamGoli Aug 1, 2017

Author Member

To Identify a formula, only classes which inherit from Formula were considered. This change adds the remaining 3 also, so that class_cop.rb's checks can run.

This comment has been minimized.

@GauthamGoli

GauthamGoli Aug 1, 2017

Author Member

It should have been class_node && class_names.include?(class_name(class_node))

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 1, 2017

Member

Gotcha. Those classes all inherit from Formula themselves, though, so perhaps it's worth looking through the entire inheritance hierarchy?

This comment has been minimized.

@GauthamGoli

GauthamGoli Aug 1, 2017

Author Member

I can't think of a way to look through inheritance hierarchy just using RuboCop without require-ing the Classes themselves. Do you think its possible?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 1, 2017

Member

If you can get the name or Class object of a class you could recurse through the inheritance hierarchy. That's maybe unnecessary, though but just an idea. I'm fine with this approach.

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_class_rubocop_port branch 2 times, most recently from cd4fd66 to 2c09000 Aug 1, 2017

end

module FormulaAuditStrict
# - `test do ..end` should be defined in a formula

This comment has been minimized.

@JCount

JCount Aug 1, 2017

Contributor

in a formula => in the formula ?

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_class_rubocop_port branch from 2c09000 to 8e1e32c Aug 1, 2017

@stale stale bot added the stale label Aug 22, 2017

@stale

This comment has been minimized.

Copy link

stale bot commented Aug 22, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@JCount

This comment has been minimized.

Copy link
Contributor

JCount commented Aug 22, 2017

@GauthamGoli I apologize, I completely lost track of this. What is the current status of this PR, do you still want this merged as-is or do you want to make modifications?

@stale stale bot removed the stale label Aug 22, 2017

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_class_rubocop_port branch from 8e1e32c to 2040fc1 Aug 23, 2017

@GauthamGoli

This comment has been minimized.

Copy link
Member Author

GauthamGoli commented Aug 23, 2017

Gauthams-MacBook-Pro:Homebrew gautham$ brew audit --only-cops=FormulaAudit/ClassName
Warning: Calling GithubGistFormula.url is deprecated!
Use Formula.url instead.
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-science/nest.rb:4:in `<class:Nest>'
Please report this to the homebrew/science tap!

homebrew/science/nest:
  * C: 1: col 14: GithubGistFormula is deprecated, use Formula instead
Error: 1 problem in 1 formula

odeprecated is being called in formula_specialities.rb in the respective methods of deprecated classes which is causing the above duplication of warning/audit errors. I kind of don't know what to do here yet.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 23, 2017

odeprecated is being called in formula_specialities.rb in the respective methods of deprecated classes which is causing the above duplication of warning/audit errors. I kind of don't know what to do here yet.

Could you rescue MethodDeprecatedError to handle this issue? I've not tried it so it may not work. If not we could consider removing this audit in favour of the message.

@GauthamGoli

This comment has been minimized.

Copy link
Member Author

GauthamGoli commented Aug 26, 2017

Why are warnings and audit rules present simultaneously for audit_class?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 27, 2017

Why are warnings and audit rules present simultaneously for audit_class?

@GauthamGoli The warnings are a runtime check to ensure that even people who don't use brew audit receive notification about their deprecation.

@GauthamGoli

This comment has been minimized.

Copy link
Member Author

GauthamGoli commented Aug 27, 2017

Okay. These warnings are piped to $stderr, and can be temporarily redirected to elsewhere when the formulae are loaded in audit to prevent the warning from appearing. But doing so would redirect whole of $stderr and other warnings would get redirected too. I'm currently trying to find some workaround.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 27, 2017

@GauthamGoli In this case it may be reasonable to modify the deprecation code to handle this case.

@GauthamGoli

This comment has been minimized.

Copy link
Member Author

GauthamGoli commented Aug 27, 2017

Modifying it to raise MethodDeprecatedError instead of piping to $stderr, and then rescueing it is tricky too, as rescue would prevent instances of formulae from getting created, in audit

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 29, 2017

Was Library/Homebrew/plist included by mistake?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 29, 2017

@GauthamGoli I reckon change

if ARGV.homebrew_developer? || disable ||
Homebrew.raise_deprecation_exceptions?
raise MethodDeprecatedError, message
else
opoo "#{message}\n"
end
to have different behaviour when running audit. For example, you could have a Homebrew.auditing? or similar which instead of raising excepts adds things to a Homebrew.audit_problems global array and then brew audit outputs them at the end. This would allow you to remove the audits that duplicate the deprecation code. Thoughts?

@GauthamGoli

This comment has been minimized.

Copy link
Member Author

GauthamGoli commented Aug 29, 2017

Yeah. Makes sense to have audit specific flag.
Instead of adding to Homebrew.audit_problems, I think its better to suppress it, and let the Cop report it because it would have access to offending node's location in the source.

Yes. Library/Homebrew/plist has been added by mistake, will remove it.

@JCount

This comment has been minimized.

Copy link
Contributor

JCount commented Aug 29, 2017

That also additionally solves the problem of having to update, (or add to), the audit code when deprecations are modified within the rest of the codebase. A big 👍 to avoiding unnecessary duplication in the codebase.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 31, 2017

Instead of adding to Homebrew.audit_problems, I think its better to suppress it, and let the Cop report it because it would have access to offending node's location in the source.

Sounds good to me 👍

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_class_rubocop_port branch from 2040fc1 to 51804af Sep 2, 2017

@GauthamGoli

This comment has been minimized.

Copy link
Member Author

GauthamGoli commented Sep 4, 2017

Made changes, can you please review?

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_class_rubocop_port branch from 51804af to 0e595bb Sep 4, 2017

@JCount

JCount approved these changes Sep 4, 2017

Copy link
Contributor

JCount left a comment

Good job! Having the global variable indicating that brew audit is being run could potentially have other uses as well. (We may end up wanting to tweak this a bit later, but that should be easy enough.) 👍

@@ -103,7 +103,7 @@ def odeprecated(method, replacement = nil, disable: false, disable_on: nil, call
Homebrew.raise_deprecation_exceptions?
raise MethodDeprecatedError, message
else

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 4, 2017

Member

elsif !Homebrew.auditing?

This comment has been minimized.

@JCount

JCount Sep 4, 2017

Contributor

Doesn't this get rid of the default case? Currently, that should not be an issue, but it does add a single point of failure for the odeprecated code message printout. (Merely personal preference on my part, nothing to block this from going forward.)

@@ -121,6 +122,7 @@ def audit
return if problem_count.zero?

ofail "#{Formatter.pluralize(problem_count, "problem")} in #{Formatter.pluralize(formula_count, "formula")}"
Homebrew.auditing = false

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 4, 2017

Member

Can probably skip setting this here as the end of a command is the end of termination, too.

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_class_rubocop_port branch from 0e595bb to d45ff9c Sep 4, 2017

@MikeMcQuaid MikeMcQuaid merged commit 4cc8d47 into Homebrew:master Sep 5, 2017

3 checks passed

codecov/patch 60.71% of diff hit (target 53.93%)
Details
codecov/project Absolute coverage decreased by -2.01% but relative coverage increased by +6.77% compared to 751334a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 5, 2017

Thanks again @GauthamGoli!

@JCount

This comment has been minimized.

Copy link
Contributor

JCount commented Sep 5, 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.