brew-audit: move style checks into main audit output #112

apjanke · 2016-04-18T21:48:16Z

Checklist

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 ran brew tests with your changes locally?

Description

Breaks up the brew style part of the check for brew audit --strict and runs it separately for each formula, when that formula is being checked for other audits. This collects all violations for each formula in a single place, instead of doing brew style outputs for all formulae first, and then the other audit checks. Easier to read. It also makes the violation count at the end of the output include the style violations.

Before:

After:

This also localizes the --fix argument support in style so that it doesn't leak to audit. I'm not sure if that's the right thing to do, though. audit isn't documented as supporting --fix, but that doesn't necessarily mean it isn't supposed to do it.

Considerations

This turns off the syntax highlighting for the Rubocop output. I think that's fine, since we're uniformly using the main audit output style here. If someone really misses it, I could add an option to enable the old behavior, with all the style checks at the top and with coloring intact. Or we could add Tty-based coloring to audit's own output, and you'll get it everywhere. (Personally, I don't care. I think this much use of color is distracting instead of helpful.)
Will be slightly slower when auditing multiple formulae due to multiple Rubocop invocations. I don't think this is a big deal: the main use case is single-file formula audits, I think. And it was still perfectly tolerable when I ran it interactively on big batches like brew audit --strict a*.
Is brew audit actually supposed to support --fix?

MikeMcQuaid · 2016-04-19T08:28:24Z

Library/Homebrew/cmd/audit.rb

+    return unless @style
+
+    style_violations = nil
+    Homebrew.check_style(@formula.path) { |rslt| style_violations = rslt }


No idea what rslt means 😉

Why not just

Homebrew.check_style(@formula.path) do |violations| violations.each { |violation| problem violation } end

Sorry; shorthand for "result". I'm still learning to remember to spell everything out for brew code.

xu-cheng's version didn't occur to me since I'm still new at Ruby. Switched to that.

MikeMcQuaid · 2016-04-19T08:28:56Z

A few comments but otherwise LGTM 👍

xu-cheng · 2016-04-19T08:45:44Z

Library/Homebrew/cmd/style.rb

-    Homebrew.failed = !$?.success?
+    if block_given?
+      txt = nil
+      Utils.popen(["rubocop", *args], "r") { |io| txt = io.read }


Utils.popen_read?

popen_read returns the results in BINARY and would require a separate force_encoding('UTF-8') call and check for Ruby 1.9+. I thought just doing a popen with a r instead of rb would be simpler.

apjanke · 2016-04-19T18:45:22Z

Switched to "in progress" while I look at switching to Rubocop's JSON output instead of parsing their normal results output.

apjanke · 2016-04-19T20:36:49Z

I rewrote the rubocop interaction using its JSON output, per @xu-cheng's suggestion. Now we're using the appropraite machine-readable format, results for multiple files are represented correctly, and the rubocop checks can be done in a single batch up front to avoid the cost of shelling out to rubocop separately for each formula.

apjanke · 2016-04-19T20:37:58Z

Question: the ==> audit problems ohai header is only displayed when --strict is given. Why is that?

MikeMcQuaid · 2016-04-20T07:03:01Z

Question: the ==> audit problems ohai header is only displayed when --strict is given. Why is that?

Could just be a mistake. Feel free to address it.

MikeMcQuaid · 2016-04-20T07:03:33Z

Library/Homebrew/cmd/audit.rb

 #:
 #:    If `--online` is passed, additional slower checks that require a network
 #:    connection are run. This should be used when creating for new formulae.
 #:
 #:    `audit` exits with a non-zero status if any errors are found. This is useful,
 #:    for instance, for implementing pre-commit hooks.

+# Undocumented options:


Looks like documentation to me 😉. Will this be enabled with --debug? If so I think it can just be omitted.

Surprisingly, no, because it takes a capital D instead of a lowercase d like --debug. I don't know why it's different, but it's a distinct option.

Weird. Given I've been working on Homebrew for a long time and had no idea I think it's worth just using ARGV.debug? instead.

I took a closer look at it. All this one does is turn on instrumentation for profiling the audit_* methods. It doesn't turn on all the other verbose debugging stuff. And since it does profiling, it probably doesn't want to enable the other debugging stuff, since that would skew the timings.

I reworded the comment and factored out that bit of code to make it more understandable.

Just wanted to mention that brew doctor has the exact same feature. It is equally undocumented.

I refactored this to share brew doctor's implementation, and added a similar "Undocumented options" comment at the top of brew doctor. Are you folks okay with the comment there? That way the -D is still hidden in the man page, but when you're looking at the source there's something to indicate the intent, which, judging from this string of comments and my own experience, was not clear.

MikeMcQuaid · 2016-04-20T07:05:15Z

Looks good to me 👍

xu-cheng · 2016-04-20T18:53:00Z

Library/Homebrew/cmd/audit.rb

+
+# Debugging support
+
+def instrument_module_with_method_timers(the_module)


It would be better to turn it to a method of FormulaAuditor like what brew doctor does.

You could also make this method the same name as used in brew doctor.

UniqMartin · 2016-04-20T21:00:42Z

Library/Homebrew/diagnostic.rb

+
+  end
+
+  def inject_dump_stats!(the_module, pattern = /.*/)


I think I prefer this to have no default for pattern. Now that this method is used by multiple commands, it should definitely be moved out of diagnostic.rb. Maybe utils.rb is a sensible place, probably even better a new file in utils/.

Yeah, that makes sense: you'd almost never actually want to do /.*/ because there are so many small methods in most modules and classes, and if you do, it should be explicit. Removed the default and moved it to Utils.

apjanke · 2016-04-20T23:52:16Z

Amended to address the latest round of output.

Two significant changes:

Before, the style checks were only run when you specified files or formulae, and were skipped when you did a plain brew audit to check everything. Now it runs style checks too, since we can specify the correct set of files.

Removed the empty line between formulae in the output (and changed to 2-space indent for the bullet lists). Martin and I think it's more readable and a better use of space this way. IMHO, especially true now that audit has been tweaked to work better for large batches of files, and now includes style output in sections that previously did not have it.

New output:

$ brew audit a* b* c* --strict
homebrew/science/abinit:
  * Use '--without-test' instead of '--without-check'. Migrate '--without-check' with `deprecated_option`.
homebrew/science/abyss:
  * Options should begin with with/without. Migrate '--enable-maxk=' with `deprecated_option`.
  * Use '--without-test' instead of '--without-check'. Migrate '--without-check' with `deprecated_option`.
homebrew/science/adam:
  * `option` (line 21) should be put before `depends_on` (line 14)
  * Description is too long. "name: desc" should be less than 80 characters.
Length is calculated as adam + desc. (currently 112)
  * Use '--without-test' instead of '--without-check'. Migrate '--without-check' with `deprecated_option`.
homebrew/science/alembic:
  * `head` (line 17) should be put before `bottle block` (line 10)
  * A `test do` test block should be added
homebrew/science/alglib:
  * Formula should have a desc (Description).
homebrew/science/alpscore:
  * Use '--with-test' instead of '--with-check'. Migrate '--with-check' with `deprecated_option`.
  * C: 27: col 25: Don't use parentheses around a method call.
homebrew/science/amos:
  * Description shouldn't start with an indefinite article (A)

The format for multi-line problems is ugly, with 2nd and later lines not being indented, like "Length..." in the following.. Going to do something about that while I'm in here.

  * Description is too long. "name: desc" should be less than 80 characters.
Length is calculated as adam + desc. (currently 112)
  * Use '--without-test' instead of '--without-check'. Migrate '--without-check' with `deprecated_option`.

UniqMartin · 2016-04-20T23:53:02Z

Library/Homebrew/cmd/audit.rb

+    return unless @style_offenses
+    display_cop_names = ARGV.include?("--display-cop-names")
+    @style_offenses.each do
+      |offense| problem offense.to_s(:display_cop_name => display_cop_names)


|offense| should be at the end of the preceding line, not at the start of this line.

apjanke · 2016-04-20T23:59:23Z

That turned out to be easier than I feared.

homebrew/science/adam:
  * `option` (line 21) should be put before `depends_on` (line 14)
  * Description is too long. "name: desc" should be less than 80 characters.
    Length is calculated as adam + desc. (currently 112)
  * Use '--without-test' instead of '--without-check'. Migrate '--without-check' with `deprecated_option`.
homebrew/science/alembic:

I think this is ready to go now, pending y'all's feedback on the output format change.

UniqMartin · 2016-04-21T00:02:46Z

Library/Homebrew/utils.rb

+      $times = {}
+      at_exit do
+        col_width = [$times.keys.map(&:size).max + 2, 15].max
+        puts $times.sort_by { |_k, v| v }.map { |method, time|


do-end for multi-line blocks please.

Same problem as before - if I change this { ... } to do ... end, it will print "Enumerator<#...>" instead of displaying the individual lines.

Maybe it's a weird parsing issue related to precedence and the fact that there are two blocks introduced in this statement?

I think it's trying to map the result of puts instead of vice-versa (http://stackoverflow.com/a/5513622); I guess you could add parentheses like ($times.sort_by ... ) or just stick with the braces.

Sorry for the nagging! Hadn't seen your other related comment. 🙈

Actually, the most sensible solution to this style issue seems to be to move the puts into the block (and to replace map with each):

$times.sort_by { |_k, v| v }.each do |method, time| puts format("%-*s %0.4f sec", col_width, "#{method}:", time) end

I already tried sticking parentheses in a couple places there, but didn't find an arrangement that worked.

Switched to Martin's suggestion.

This collects all violations for each formula in a single place, instead of doing `brew style` outputs for all formulae first, and then the other audit checks.

MikeMcQuaid · 2016-04-21T08:06:11Z

Looking good to me 👍. We can always iterate after shipping.

UniqMartin · 2016-04-21T16:29:04Z

👍 I can think of a few minor improvements, but they can wait and I've pestered you enough in this PR. I'm really happy with the code now (and the major improvement to brew audit hidden therein).

What I'd really like you to consider for future PRs (or even this PR, if you feel like retroactively doing this) would be not to lump all changes into a single commit. It's really hard to digest once some time has passed and you're looking at the Git history. Looking at the current code, here's what the theme of individual commits could be (they should be all independent and build on top of each other):

utils: add popen_read_text
utils: refactor inject_dump_stats! to be independent of Homebrew::Diagnostic::Checks (and adapt cmd/doctor.rb, diagnostic.rb accordingly).
style: refactor and use RuboCop JSON interface
style: add documentation
audit: use generalized inject_dump_stats!
audit: better integrate style checks

apjanke · 2016-04-21T18:00:40Z

Sorry! I've gotten used to squashing to a single commit to formula changes; maybe overapplied it here.

I could still break these out to multiple commits if you think it would help readability. (Some git clients I use make that pretty easy; maybe they foresaw this situation.)

UniqMartin · 2016-04-21T18:09:00Z

Sorry! I've gotten used to squashing to a single commit to formula changes; maybe overapplied it here.

No need to apologize! It's just something that I noticed and I wanted to make you aware of. Just pretend for a moment that there's a breaking change in audit (very unlikely) introduced by this code. It would be a shame to have to revert all the other improvements also contained in the same commit and then again work on a fairly big chunk to get things sorted out.

I could still break these out to multiple commits if you think it would help readability. (Some git clients I use make that pretty easy; maybe they foresaw this situation.)

I'd leave this decision up to you. It's obviously easier to do when things get split right from the start. I'm also fine merging it as-is, as nobody except me complained and even I did so at the very last moment.

apjanke · 2016-04-21T18:43:58Z

Turns out splitting this would take a good amount of work: I'd want to make sure each individual commit still worked, so it would take manual editing and several rounds of testing, and I'm short on time this week. Let's just merge as-is, and I'll work on organizing future PRs better.

apjanke · 2016-04-21T18:45:56Z

Shipped. Thanks for your feedback, folks!

apjanke changed the title ~~brew-audit: pull style checks in to main audit output~~ brew-audit: move style checks into main audit output Apr 18, 2016

apjanke added features usability Usability of Homebrew/brew labels Apr 18, 2016

MikeMcQuaid reviewed Apr 19, 2016
View reviewed changes

xu-cheng reviewed Apr 19, 2016
View reviewed changes

apjanke added the in progress Maintainers are working on this label Apr 19, 2016

apjanke removed the in progress Maintainers are working on this label Apr 19, 2016

MikeMcQuaid reviewed Apr 20, 2016
View reviewed changes

xu-cheng reviewed Apr 20, 2016
View reviewed changes

UniqMartin reviewed Apr 20, 2016
View reviewed changes

apjanke added the in progress Maintainers are working on this label Apr 20, 2016

UniqMartin reviewed Apr 20, 2016
View reviewed changes

apjanke removed the in progress Maintainers are working on this label Apr 20, 2016

UniqMartin reviewed Apr 21, 2016
View reviewed changes

brew-audit: pull style checks in to main audit output

abe1379

This collects all violations for each formula in a single place, instead of doing `brew style` outputs for all formulae first, and then the other audit checks.

apjanke closed this Apr 21, 2016

apjanke reopened this Apr 21, 2016

apjanke closed this in a3b70d3 Apr 21, 2016

apjanke deleted the audit-style-per-file branch April 21, 2016 19:24

This was referenced May 3, 2016

Feature Request: Preventing formulae updates from breaking their dependencies #60

Closed

audit --strict output divided by topic, not formula Homebrew/legacy-homebrew#44606

Closed

Homebrew locked and limited conversation to collaborators May 3, 2018


		# Debugging support

		def instrument_module_with_method_timers(the_module)

Uh oh!

brew-audit: move style checks into main audit output #112

brew-audit: move style checks into main audit output #112

Uh oh!

Conversation

apjanke commented Apr 18, 2016

Checklist

Description

Considerations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid commented Apr 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apjanke commented Apr 19, 2016

Uh oh!

apjanke commented Apr 19, 2016

Uh oh!

apjanke commented Apr 19, 2016

Uh oh!

MikeMcQuaid commented Apr 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid commented Apr 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apjanke commented Apr 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apjanke commented Apr 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid commented Apr 21, 2016

Uh oh!

UniqMartin commented Apr 21, 2016

Uh oh!

apjanke commented Apr 21, 2016

Uh oh!

UniqMartin commented Apr 21, 2016

Uh oh!