Skip to content
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

feat: Rule Performance Statistics for flat ESLint #17850

Merged
merged 55 commits into from Mar 25, 2024
Merged

Conversation

mnkiefer
Copy link
Contributor

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[X] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

  • Introduces a new flag --stats to flat ESLint, which enables the user to obtain a series of runtime statistics on top of their final lint results (refer to the table below).
  • See RFC

Is there anything you'd like reviewers to focus on?

@mnkiefer mnkiefer requested a review from a team as a code owner December 13, 2023 16:27
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Dec 13, 2023
Copy link

netlify bot commented Dec 13, 2023

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 5b37bce
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6600724b88bb0e0008d9a435
😎 Deploy Preview https://deploy-preview-17850--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@nzakas nzakas marked this pull request as draft December 13, 2023 16:31
@nzakas
Copy link
Member

nzakas commented Dec 13, 2023

Just converting this to a draft so we know it's not yet time to review it.

lib/shared/stats.js Outdated Show resolved Hide resolved
lib/linter/linter.js Outdated Show resolved Hide resolved
lib/shared/types.js Outdated Show resolved Hide resolved
lib/linter/linter.js Outdated Show resolved Hide resolved
lib/linter/linter.js Outdated Show resolved Hide resolved
lib/linter/timing.js Outdated Show resolved Hide resolved
lib/shared/stats.js Outdated Show resolved Hide resolved
@nzakas
Copy link
Member

nzakas commented Jan 2, 2024

@me4502 getting back around to this now. Can you please rebase on top of main? And if you feel you're ready for review now, please feel free to hit that button. :)

@mnkiefer
Copy link
Contributor Author

mnkiefer commented Jan 3, 2024

Getting back around to this now. Can you please rebase on top of main? And if you feel you're ready for review now, please feel free to hit that button. :)

Great to hear! I just did a rebase, so the implementation should be up to date with the new ESLint now. All that is left is to add tests for it, but I suppose those would come in a second review round? So, pressing the Ready for Review button now 👍

@mnkiefer mnkiefer marked this pull request as ready for review January 3, 2024 09:25
@nzakas
Copy link
Member

nzakas commented Jan 5, 2024

Go ahead and add the tests now. We like to review the code and the tests together as oftentimes the tests tell more of the story than the code does.

lib/linter/linter.js Outdated Show resolved Hide resolved
tests/lib/eslint/eslint.js Outdated Show resolved Hide resolved
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good. I'd just like to avoid passing stats around in so many places. (I left a comment inline about this, as well.) If we can instead pull that parts out of slots and later assign them back, we can make the additional functionality more testable.

lib/linter/linter.js Show resolved Hide resolved
lib/shared/stats.js Show resolved Hide resolved
lib/shared/stats.js Outdated Show resolved Hide resolved
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refactoring around slots looks good to me. Left a couple of suggestions for areas to clean up, but this is looking really good.

lib/linter/linter.js Outdated Show resolved Hide resolved
lib/linter/linter.js Outdated Show resolved Hide resolved
lib/linter/linter.js Outdated Show resolved Hide resolved
lib/shared/stats.js Outdated Show resolved Hide resolved
@mnkiefer mnkiefer requested a review from nzakas January 21, 2024 07:14
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Would like @mdjermanovic to review before merging.

lib/eslint/eslint.js Outdated Show resolved Hide resolved
@amareshsm amareshsm added the accepted There is consensus among the team that this change meets the criteria for inclusion label Feb 1, 2024
nzakas
nzakas previously approved these changes Feb 9, 2024
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest changes LGTM. Waiting for @mdjermanovic to re-review.

@mnkiefer
Copy link
Contributor Author

Latest changes LGTM. Waiting for @mdjermanovic to re-review.

@nzakas: Any updates on this? 😇

@mdjermanovic
Copy link
Member

@mnkiefer sorry for the delay, I'll review it soon.

@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features contributor pool labels Feb 21, 2024
docs/src/use/command-line-interface.md Outdated Show resolved Hide resolved
lib/shared/types.js Outdated Show resolved Hide resolved
lib/shared/types.js Outdated Show resolved Hide resolved
@mdjermanovic
Copy link
Member

Totals for rules should be the same as with TIMING? I'm getting lower numbers with --stats than with TIMING.

For example:

// eslint.config.js
module.exports = [{
    languageOptions: {
        sourceType: "commonjs"
    },
    linterOptions: {
        reportUnusedDisableDirectives: "off"
    },
    rules: {
        indent: ["error", 4, { SwitchCase: 1 }]
    }
}];
$ TIMING=all node bin/eslint tests/lib/linter/linter.js --stats -f json
(node:7892) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignoring-files
(Use `node --trace-warnings ...` to show where the warning was created)
[{"filePath":"C:\\projects\\tmp\\mnkiefer-eslint\\tests\\lib\\linter\\linter.js","messages":[],"suppressedMessages":[],"errorCount":0,"fatalErrorCount":0,"warningCount":0,"fixableErrorCount":0,"fixablea 
rningCount":0,"stats":{"times":{"passes":[{"parse":{"total":251.2441},"rules":{"indent":{"total":182.5374}},"fix":{"total":0},"total":619.8087}]},"fixPasses":0},"usedDeprecatedRules":[{"ruleId":"indent","replacedBy":[]}]}]
Rule   | Time (ms) | Relative
:------|----------:|--------:
indent |   214.375 |   100.0%

Per the stats, indent rule takes 182.5374ms, but TIMING in the same run outputs 214.375ms.

lib/linter/linter.js Outdated Show resolved Hide resolved
@github-actions github-actions bot added the cli Relates to ESLint's command-line interface label Feb 21, 2024
mnkiefer and others added 9 commits March 18, 2024 19:08
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
tests/lib/eslint/eslint.js Outdated Show resolved Hide resolved
docs/src/integrate/nodejs-api.md Outdated Show resolved Hide resolved
docs/src/integrate/nodejs-api.md Outdated Show resolved Hide resolved
mnkiefer and others added 6 commits March 22, 2024 16:36
mdjermanovic
mdjermanovic previously approved these changes Mar 22, 2024
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Leaving open for @nzakas to verify again as this has had a lot of changes since the last approval.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just left a few notes of things to clean up before merging.

lib/linter/linter.js Outdated Show resolved Hide resolved
docs/src/extend/stats.md Outdated Show resolved Hide resolved
docs/src/extend/stats.md Outdated Show resolved Hide resolved
docs/src/extend/stats.md Outdated Show resolved Hide resolved
docs/src/extend/stats.md Outdated Show resolved Hide resolved
lib/linter/linter.js Outdated Show resolved Hide resolved
docs/src/integrate/nodejs-api.md Show resolved Hide resolved
mnkiefer and others added 8 commits March 23, 2024 11:28
Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Once again, thank you for your hard work on such a detailed feature. We really appreciate it. 🙏

@nzakas nzakas merged commit de40874 into eslint:main Mar 25, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion cli Relates to ESLint's command-line interface contributor pool core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

6 participants