-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add eslint-plugin-regexp #142
Merged
Merged
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
18d2a0e
Add eslint-plugin-regexp
ybiquitous 7d67470
Disable `regexp/prefer-d`
ybiquitous f5764bc
Merge branch 'master' into add-eslint-plugin-regexp
ybiquitous 5b627bf
Update package-lock.json
ybiquitous 9047b8f
Enable `regexp/no-unused-capturing-group`
ybiquitous File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
[note] There is room for discussion about disabling
regexp/no-unused-capturing-group
. Please give me feedback if any.See also #141 (comment)
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.
?:
is readable...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.
0-9
vs\d
is not readableThere 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.
@alexander-akait Thanks for the feedback!
Did you mean we should enable
regexp/no-unused-capturing-group
?Did you mean we should disable
prefer-d
? This rule is for code style, so it seems acceptable to me that we can disable it.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.
Yes, I found perf is better here,
?:
is not hard to readIt is just my opinion, better to use explicit in some cases
/cc @hudochenkov @jeddy3
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.
About
prefer-d
, the following demo in stylelint/stylelint#5516 may be helpful.https://github.com/stylelint/stylelint/pull/5516/files#diff-bc86d8e365c9e39833c23e23b86683913daa42ccf65be917f15f1cab7f3c6659
Personally, I feel
[0-9A-Za-z]
is a bit more readable than[\dA-Za-z]
.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.
Let's turn off
prefer-d
for now. We can always revisit.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.
Done 7d67470.
What about
regexp/no-unused-capturing-group
?https://github.com/stylelint/stylelint/pull/5516/files#diff-0aada42ad688056595a24011819e7047990bbcf88104577fc706851596ce7263
In this case,
?:
surely seems a bit hard to read, but performance is important too.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.
Let's favour performance for now. We can always change back if it proves an issue.
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.
@jeddy3 Thanks for the advice. Done 9047b8f. 👍🏼