-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(rule-tester): add reimplementation of RuleTester #9331
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
Conversation
Introduces a `FlatRuleTester` class which overhauls the API Why? Because we shouldn't be writing rules as JSON inside our (Java/Type)script And the new API prevents writing our tests as a giant function call which we pass a giant object We'll need to port all the rules over to the new tester eventually
Thanks for the PR, @luca-script! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luca-script sorry, I'm very confused here. As I understood it, the backing ESLint PR eslint/eslint#17922 was mostly just a rename. From its OP:
What changes did you make? (Give an overview)
- Removed
RuleTester
- Renamed
FlatRuleTester
toRuleTester
- Updated
api.js
to export the newRuleTester
- Updated
unsupported-api.js
to stop exportingFlatRuleTester
- Updated the API docs
- Updated the Migrate to v9 guide
- Updated all rule tests
...and our issue #9017 is just for us to do a similar rename.
Why the reimplementation of FlatRuleTester
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure. We don't want a complete reimplementation for scratch because we are looking for feature parity with upstream.
The existing implementation is intended to be as close to the original code as possible (warts and all) to make it easy for us to merge upstream changes back in to our fork.
Not that we need parity because it's intended to be a drop-in replacement - an augmentation of the core rule tester.
I don't much care for their code style either - and I would love to rewrite it from scratch - but that will make maintenance harder in the long run.
👋 @luca-script is this still something you have time + energy for? |
@JoshuaKGoldberg Is this feature only planned for the next major release? |
Yes: it's a breaking change so we can't get it into v7. |
Closing this PR out as I'm tackling the backing issue with #9603. @luca-script you did a lot of great work in this PR and I'm sad to see it closed. Same as #9346. Next time, I'd suggest reading the issue carefully & checking with us before you put a lot of effort into a big PR. My advice in general is to ease into a new repository with smaller bugfixes & not-exceptionally-complex feature additions before tackling big issues like these ones. ❤️ |
PR Checklist
Overview
Adds a new
FlatRuleTester
implementation, which supports flat config and has been completely (from scratch) rewritten in full TypeScript with greater performance and following more modern coding standards (Namely defining your rules as code as opposed to passing a giant JSON-esque array)Porting should be easy via the
FlatRuleConfig.fromObject
method (Which takes a mostlyRuleTester.run
-compatible object)And should be more performant (Although I have not checked yet)
BREAKING CHANGES COMPARED TO
RuleTester
:TODO
/// <reference types="...">
)@typescript-eslint/parser
as the parserLinter
types