-
-
Notifications
You must be signed in to change notification settings - Fork 687
refactor: use regexp-groups to simplify ignores #2776
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
|
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.
I like the idea! It would be great to see it used a lot more in the whole codebase 🙂
Co-authored-by: Flo Edelmann <[email protected]>
Does that count as
so that I should start converting the rest of the applicable locations? |
That would be great! Do you want to want to give it a go? |
Ready for review/merge. There are a few locations left, that either use plain string[] ignore lists or just use a single ignore pattern, that I haven't touched, but I think they can be enhanced in later PRs on demand. |
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.
Very cool, thank you! 🙂
Looks good to me now.
There are a few locations left, that either use plain string[] ignore lists or just use a single ignore pattern, that I haven't touched, but I think they can be enhanced in later PRs on demand.
Would you like to submit a follow-up PR for this? That would be awesome! 🚀
Don't be surprised, I'll try out the Copilot reviewer feature now 😄 |
Co-authored-by: Flo Edelmann <[email protected]>
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.
LGTM! Thank you!
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.
Pull Request Overview
This PR introduces a new helper toRegExpGroupMatcher
to streamline ignore/allow logic by consolidating multiple string[] → RegExp[] → boolean
patterns into a single matcher function. It also extends toRegExp
to accept flag additions/removals and refactors existing rule files to use the new helper.
- Adds
toRegExpGroupMatcher
and updatestoRegExp
signature inlib/utils/regexp.js
- Replaces many
.map(toRegExp)
and custombuildMatcher
usages in rule files withtoRegExpGroupMatcher
- Adds comprehensive tests for both
toRegExp
andtoRegExpGroupMatcher
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
tests/lib/utils/regexp.js | Expanded tests for toRegExp and new toRegExpGroupMatcher |
lib/utils/regexp.js | Added toRegExpGroupMatcher , enhanced toRegExp with flag handling |
lib/rules/* | Refactored ignore/allow patterns to use toRegExpGroupMatcher and removed custom matcher logic |
Comments suppressed due to low confidence (2)
lib/utils/regexp.js:56
- The JSDoc for toRegExpGroupMatcher could be expanded to note that input patterns may be regex-like strings (e.g., '/^foo/i') and that flags are handled by the underlying toRegExp helper.
* Converts an array of strings to a singular function to match any of them.
lib/utils/regexp.js:62
- [nitpick] The name 'toRegExpGroupMatcher' is quite verbose; consider a shorter, more intention-revealing name (e.g., 'matchAnyPattern' or 'anyPatternMatcher') to improve readability.
function toRegExpGroupMatcher(patterns = []) {
Should we mention refactorings in the changelog? If no, then no changeset is necessary. |
Make sense to me, users don’t really care about internal code shuffling, so we don’t need to include it. |
Implement idea suggested here: #2773 (comment)
Excluding/Ignoring things is a common task in rules. This PR creates a helper that hides all the string[] -> regexp[] -> boolean logic in a simple helper function.
The current project contains at least 9 instances of
.map(toRegExp)
+ a few more using more verbose syntax.If the general design of this feature is accepted, I will convert the other locations to the new feature and mark this PR as
POC.I'm not super happy with the method name and jsdocs, so any input would be appreciated.