feat(vitest): [titleValidity] add rule - #3191
Conversation
🦋 Changeset detectedLatest commit: 1c6177e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for flint-fyi ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This comment has been minimized.
This comment has been minimized.
michaelfaith
left a comment
michaelfaith
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Some feedback from my side.
| "@flint.fyi/vitest": patch | ||
| --- | ||
|
|
||
| [titleValidity] add rule. |
There was a problem hiding this comment.
| [titleValidity] add rule. | |
| Added `titleValidity` rule, which aims to enforce valid titles for `describe()`, `it()` and `test()` titles. |
| declare const value: unknown; | ||
|
|
||
| describe(typeof value, () => { | ||
| // ... | ||
| }); |
There was a problem hiding this comment.
This example is not what the vitest equivalent option permits (or what the description above describes.
| Not set by default. | ||
|
|
||
| A string or a `[pattern, message]` pair applies to `describe`, `it`, and `test` alike. | ||
| An object applies a separate pattern per function: |
There was a problem hiding this comment.
| An object applies a separate pattern per function: | |
| Using an object with `describe`, `it`, and/or `test` properties, you can apply a different match pattern for each function: |
| Regular expressions that titles must match, optionally with a custom message. | ||
| Not set by default. | ||
|
|
||
| A string or a `[pattern, message]` pair applies to `describe`, `it`, and `test` alike. |
There was a problem hiding this comment.
Is there a reason we've deviated from the API of the eslint equivalent of this option? I.e. having a pattern/message array instead of having an array of regexes?
| ], | ||
| }); | ||
|
|
||
| ruleTester.describe(rule, { |
There was a problem hiding this comment.
Why are there multiple calls to ruleTester.describe? Each rule should have one top-level describe.
| message: mustNotMatch.message ? "mustNotMatchCustom" : "mustNotMatch", | ||
| range, | ||
| }); | ||
| return; |
There was a problem hiding this comment.
Why return here? If the title both matches something it shouldn't and doesn't match something it should, wouldn't we want to report both? Rather than only reporting one, they fix that, and then we report the other...
| const [pattern, message] = | ||
| typeof matcher === "string" ? ([matcher] as const) : matcher; | ||
|
|
||
| return { message, pattern: new RegExp(pattern, "u") }; |
There was a problem hiding this comment.
I'd like to cache these RegExp objects. We'll end up remaking RegExp objects with the same patterns, over and over and over again, which has a non-trivial impact over time.
| const disallowedMatch = new RegExp( | ||
| `\\b(${options.disallowedWords.join("|")})\\b`, | ||
| "iu", | ||
| ).exec(title); |
There was a problem hiding this comment.
We're just creating this RegExp using it and then throwing it away. Can we not create it in the setup scope and reuse it with every call?
| const trimmed = text | ||
| .replace(/^(['"`])\s+/u, "$1") | ||
| .replace(/\s+(['"`])$/u, "$1"); |
There was a problem hiding this comment.
Seems like this could be simplified into a single regex (and that regex pulled out to the module scope so we're only creating one instance). And would this be easier by just identifying which quote type the original used and surround title.trim() with those?
| presets: ["logical", "logicalStrict"], | ||
| }, | ||
| messages: { | ||
| accidentalSpace: { |
There was a problem hiding this comment.
| accidentalSpace: { | |
| unnecessarySpace: { |
We don't know whether it's accidental or not. But we do know it's unnecessary
michaelfaith
added
the
status: waiting for author
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
3 participants
PR Checklist
titleValidityrule #2696status: accepting prsOverview
The implementation and tests closely mirror the Vitest implementation.
For review: