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.

Sign up for GitHub

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

Jump to bottom

Add eslint-plugin-regexp #142

Merged
merged 5 commits into from Sep 14, 2021
Merged

Add eslint-plugin-regexp #142

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
18d2a0e
Add eslint-plugin-regexp
ybiquitous Sep 14, 2021
7d67470
Disable `regexp/prefer-d`
ybiquitous Sep 14, 2021
f5764bc
Merge branch 'master' into add-eslint-plugin-regexp
ybiquitous Sep 14, 2021
5b627bf
Update package-lock.json
ybiquitous Sep 14, 2021
9047b8f
Enable `regexp/no-unused-capturing-group`
ybiquitous Sep 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions .eslintrc.js
View file
Open in desktop
Expand Up @@ -15,6 +15,7 @@ module.exports = {
"plugin:node/recommended",
"plugin:jest/recommended",
"plugin:jest/style",
"plugin:regexp/recommended",
"prettier",
],
rules: {
Expand Down Expand Up @@ -113,5 +114,8 @@ module.exports = {
"prefer-template": "error",
"sort-requires/sort-requires": "error",
strict: ["error", "global"],

// Prefer code readability over a bit performance improvement.
"regexp/no-unused-capturing-group": "off",
Copy link
Member Author

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)

Copy link
Member

Choose a reason for hiding this comment

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

?: is readable...

Copy link
Member

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 readable

Copy link
Member Author

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!

?: is readable...

Did you mean we should enable regexp/no-unused-capturing-group?

0-9 vs \d is not readable

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.

Copy link
Member

Choose a reason for hiding this comment

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

Did you mean we should enable regexp/no-unused-capturing-group?

Yes, I found perf is better here, ?: is not hard to read

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.

It is just my opinion, better to use explicit in some cases

/cc @hudochenkov @jeddy3

Copy link
Member Author

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.

image

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].

-const HEX = /^#[0-9A-Za-z]+/;
+const HEX = /^#[\dA-Za-z]+/;

Copy link
Member

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.

Copy link
Member Author

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.

Done 7d67470.

What about regexp/no-unused-capturing-group?

image

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.

-const HEX = /^#([\da-f]{3,4}|[\da-f]{6}|[\da-f]{8})$/i;
+const HEX = /^#(?:[\da-f]{3,4}|[\da-f]{6}|[\da-f]{8})$/i;

Copy link
Member

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.

Copy link
Member Author

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. 👍🏼

},
};
174 changes: 168 additions & 6 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -29,6 +29,7 @@
"eslint-plugin-eslint-comments": "^3.2.0",
"eslint-plugin-jest": "^24.3.6",
"eslint-plugin-node": "^11.1.0",
"eslint-plugin-regexp": "^1.1.0",
"eslint-plugin-sort-requires": "^2.1.0"
},
"publishConfig": {
Expand Down