dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.28k stars 1.59k forks source link

Timings script should highlight slow lint rules #57765

Open srawlins opened 6 years ago

srawlins commented 6 years ago

I think we could colorize the expanded output (behind a flag?):

/Users/srawlins/code/dart-linter/foo/lib/src/foo/dir/x.dart 1:8 [lint] Document all public members.
String addddddd() => 'yay';
       ^^^^^^^^

1 file analyzed, 1 issue found, in 6472 ms.

Maybe colorize the text being indicated red? addddddd? Or make it bold?

And the --stats output, as part of this TODO:

https://github.com/dart-lang/linter/blob/a088573c6628935a634d642d0863b7fd58b106a6/lib/src/formatter.dart#L96

we could also colorize/shame slow linter rules

------------------------------------------------------
Timings                                             ms
------------------------------------------------------
public_member_api_docs                             180
invariant_booleans                                   7
directives_ordering                                  4
non_constant_identifier_names                        2
file_names                                           1
test_types_in_equals                                 0
------------------------------------------------------
Total                                               32
------------------------------------------------------

Would it be acceptable to add a dependency on ansicolor and start with minimal colors (my two examples)?

pq commented 6 years ago

I love the idea of colorizing output (and am a big fan of ansicolor). One concern is that, in general, we're trying to steer folks away from depending too much on the linter binary itself, preferring the dartanalyzer as the single front-end for linting. (See dart-lang/sdk#57427 for some discussion but feel free to ping me for more context.) It'd be more work obviously, but I wonder if we could consider adding color to the analyzer CLI?

/cc @bwilkerson

bwilkerson commented 6 years ago

I'm not sure why it would be more work, but yes, the command-line analyzer is the right place for this work.

srawlins commented 6 years ago

Ah I see, certainly. What brought me here actually, was the TODO on the --strict flag. The TODO doesn't specify an idea on how to shame a slow lint rule. I had the idea to color it red. Are there other ideas?

------------------------------------------------------
Timings                                             ms
------------------------------------------------------
public_member_api_docs                             180!!!
invariant_booleans                                 180 SHAME
directives_ordering                                180 (that's way slow; consider disabling)
------------------------------------------------------
Total                                              720
------------------------------------------------------
pq commented 6 years ago

I'm not sure why it would be more work, but yes, the command-line analyzer is the right place for this work.

OK. I guess you're right. Not more, just different... 😄

Are there other ideas?

I'm not sure. At the moment, I think very few folks look at the benchmarking bot so coloring may not be quite enough anyway. I guess it comes down to what identifying slow lints would do for us. Ideally I think this information would be most useful if tied back to the lint description so that folks can use timings as additional criteria when choosing a lint rule set. I guess another potential would be if we tracked trends or captured a baseline so that we could flag regressions --- in this case by failing the build. Other ideas?

srawlins commented 6 years ago

Yeah I think that adding a group "expensive" or "slow" will help the user. Maybe --stats doesn't need anything more then.