New: Configurable List Size For Per-Rule Performance Metrics #66
Conversation
nzakas
added
Initial Commenting
There was a problem hiding this comment.
Thanks for submitting this RFC. I can see how it would be useful to output performance information about more rules. The one thing that's missing from this RFC is the detail design, which should describe how the feature will be implemented. So far, you've done a good job of describing how the end user would make use of this change (and I do prefer staying with TIMING rather than adding another environment variable). The next step would be to dig into the code to see where you'd need to make changes and then update this RFC with those details.
|
@nzakas thanks for the tip, I have added a code sample to the RFC. |
| | `eslint` | do not show | | ||
| | `TIMING=0 eslint` | show the first 10 rules (due to minimum of 10) | | ||
| | `TIMING=1 eslint` | show the first 10 rules (due to minimum of 10) | | ||
| | `TIMING=5 eslint` | show the first 10 rules (due to minimum of 10) | | ||
| | `TIMING=10 eslint` | show the first 10 rules | | ||
| | `TIMING=11 eslint` | show the first 11 rules | | ||
| | `TIMING=15 eslint` | show the first 15 rules | | ||
| | `TIMING=100 eslint` | show the first 100 rules | | ||
| | `TIMING=all eslint` | show all the rules | |
There was a problem hiding this comment.
Should we account for other values that cannot be parsed? I've seen TIMING=true usage in issues, and it currently works the same as TIMING=1.
There was a problem hiding this comment.
Added TIMING=true to the list.
| const MINIMUM_SIZE = 10; | ||
| const TIMING_ENV_VAR_AS_INTEGER = Number.parseInt(process.env.TIMING, 10); | ||
|
|
||
| return Math.max(MINIMUM_SIZE, TIMING_ENV_VAR_AS_INTEGER); |
There was a problem hiding this comment.
For TIMING=true, this function will return NaN and display will show nothing as slice returns [] if the second argument was NaN.
There was a problem hiding this comment.
Thanks, I fixed for the code for this case. Will also add test cases in the actual PR.
|
I like this 👍 Good idea @bmish. Thanks for catching those edge cases, @mdjermanovic. If it's not |
|
Thanks for the feedback! Addressed. |
|
Status update: we still need to leave this PR open for public comment for another nine days per our RFC process. Then we go into final commenting for seven days. It looks like there’s good alignment on the team, though, so I don’t expect any hiccups along the way. |
btmills
added
Final Commenting
|
This has now been open for more than 21 days and currently has no unresolved questions, so I'm moving it to final commenting. |
|
No further issues were brought up regarding this RFC during the final commenting period, so marking this RFC as accepted and merging it. |
Summary
Running
TIMING=1 eslintfrom the command-line outputs a list of per-rule performance metrics. The list is currently limited to the top 10 longest-running rules. We would like to enable users to see a longer list of rules if desired.Open Questions
It was difficult choosing between the recommended design (reusing the
TIMINGvariable) and Alternative A (adding a newTIMING_LIMITvariable). Interested to hear which one people prefer.Related Issues
eslint/eslint#13671