Update: Add exceptionPatterns to id-length rule (fixes #13094) #13099

burawi · 2020-03-26T18:04:07Z

Prerequisites checklist

I have read the contributing guidelines.
The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

#13094

What changes did you make? (Give an overview)

I added an option named exceptionPatterns to id-length, which takes as value an array of the allowed patterns.
If the option is not defined it takes an empty array as default value.
Then I declared a boolean matchesExceptions that gets true if one of the patterns tests the name.
And added that boolean to the condition.

I wrote 2 tests:

A valid one that uses exceptionPatterns option to allow the long identifier name.
And another one, that would be invalid in which I put the same data as the previous test but this time without the new option.

Is there anything you'd like reviewers to focus on?

No

anikethsaha

there are two un-necessary files which need to be revert.

Also, the referred issue is not labeled as Accepted so I guess, this needs to be hold till that gets accepted

mdjermanovic · 2020-05-16T04:44:38Z

Marking as accepted since the issue is accepted now.

mdjermanovic

Thanks for the PR! I left some notes about optimization and testing.

This change also needs a documentation update in docs/rules/id-length.md

mdjermanovic · 2020-05-21T06:23:20Z

lib/rules/id-length.js

+                const matchesExceptions = exceptionPatterns.some(
+                    pattern => new RegExp(pattern, "u").test(name)
+                );


Perhaps we should extract this to a function and call that function in the if below, to avoid creating new RegExp instances (and testing) for each identifier in the source code, including those that have a valid length.

We could also map the input string array to a RegExp array right away in create(context)

mdjermanovic · 2020-05-21T06:27:01Z

tests/lib/rules/id-length.js

+            parserOptions: { ecmaVersion: 6 },
+            errors: [
+                tooLongError
+            ]


Can we also add an invalid test with an identifier that doesn't match configured pattern?

mdjermanovic · 2020-05-21T06:34:20Z

tests/lib/rules/id-length.js

@@ -77,7 +77,8 @@ ruleTester.run("id-length", rule, {
        { code: "var {x} = foo;", options: [{ properties: "never" }], parserOptions: { ecmaVersion: 6 } },
        { code: "var {x, y: {z}} = foo;", options: [{ properties: "never" }], parserOptions: { ecmaVersion: 6 } },
        { code: "let foo = { [a]: 1 };", options: [{ properties: "always" }], parserOptions: { ecmaVersion: 6 } },
-        { code: "let foo = { [a + b]: 1 };", options: [{ properties: "always" }], parserOptions: { ecmaVersion: 6 } }
+        { code: "let foo = { [a + b]: 1 };", options: [{ properties: "always" }], parserOptions: { ecmaVersion: 6 } },
+        { code: "function BEFORE_send() {};", options: [{ min: 3, max: 5, exceptionPatterns: ["^BEFORE_"] }], parserOptions: { ecmaVersion: 6 } }


parserOptions isn't necessary here.

mdjermanovic · 2020-05-21T06:37:09Z

tests/lib/rules/id-length.js

+        {
+            code: "function BEFORE_send() {};",
+            options: [{ min: 3, max: 5 }],
+            parserOptions: { ecmaVersion: 6 },


parserOptions doesn't seem to be necessary here.

mdjermanovic · 2020-05-21T07:02:18Z

tests/lib/rules/id-length.js

@@ -77,7 +77,8 @@ ruleTester.run("id-length", rule, {
        { code: "var {x} = foo;", options: [{ properties: "never" }], parserOptions: { ecmaVersion: 6 } },
        { code: "var {x, y: {z}} = foo;", options: [{ properties: "never" }], parserOptions: { ecmaVersion: 6 } },
        { code: "let foo = { [a]: 1 };", options: [{ properties: "always" }], parserOptions: { ecmaVersion: 6 } },
-        { code: "let foo = { [a + b]: 1 };", options: [{ properties: "always" }], parserOptions: { ecmaVersion: 6 } }
+        { code: "let foo = { [a + b]: 1 };", options: [{ properties: "always" }], parserOptions: { ecmaVersion: 6 } },
+        { code: "function BEFORE_send() {};", options: [{ min: 3, max: 5, exceptionPatterns: ["^BEFORE_"] }], parserOptions: { ecmaVersion: 6 } }
    ],


Can we also add a valid test with multiple exception patterns?

kaicataldo · 2020-06-13T00:11:45Z

@burawi Are you still interested in working on this?

burawi · 2020-07-07T09:10:47Z

@burawi Are you still interested in working on this?

I'm sorry, I'm a little bit busy currently I can't invest time in this

aladdin-add · 2020-08-14T16:12:58Z

Closing because there hasn't been activity for 30 days. If you're still interested in submitting this code, please feel free to resubmit.

* Add #13099 to continue * Delete unnecessary 'parserOptions' * Add invalid test with an identifier that doesn't match configured pattern * Add valid test with multiple exception patterns * Add a function that extracted 'return' below * Docs: Add "exceptionPatterns" to "id-length" rule * Add a function that extracted 'return' below Docs: Add "exceptionPatterns" to "id-length" rule * Update : modify wrong example * Update: all RegExp instance create in "create(context)" * Update: more simpler by refactoring some codes * Update docs/rules/id-length.md Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * Update: modify the function name * Update: Add a valid test with multiple patterns where the first doesn't match. Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update: Add exceptionPatterns to id-length rule (fixes eslint#13094)

bd99987

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Mar 26, 2020

anikethsaha suggested changes Mar 26, 2020

View reviewed changes

Delete unnecessary files

b8d3b4f

yeonjuan added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Apr 24, 2020

anikethsaha mentioned this pull request Apr 24, 2020

id-length exceptions pattern #13094

Closed

mdjermanovic added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed accepted There is consensus among the team that this change meets the criteria for inclusion labels Apr 25, 2020

mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels May 16, 2020

mdjermanovic linked an issue May 16, 2020 that may be closed by this pull request

id-length exceptions pattern #13094

Closed

mdjermanovic requested changes May 21, 2020

View reviewed changes

aladdin-add closed this Aug 14, 2020

sodaMelon added a commit to sodaMelon/eslint that referenced this pull request Aug 15, 2020

Add eslint#13099 to continue

f96b1a7

sodaMelon mentioned this pull request Aug 17, 2020

Update: Add exceptionPatterns to id-length rule (fixes #13094) #13576

Merged

1 task

eslint-deprecated bot locked and limited conversation to collaborators Feb 11, 2021

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: Add exceptionPatterns to id-length rule (fixes #13094) #13099

Update: Add exceptionPatterns to id-length rule (fixes #13094) #13099

burawi commented Mar 26, 2020

anikethsaha left a comment

mdjermanovic commented May 16, 2020

mdjermanovic left a comment

mdjermanovic May 21, 2020

mdjermanovic May 21, 2020

mdjermanovic May 21, 2020

mdjermanovic May 21, 2020

mdjermanovic May 21, 2020

kaicataldo commented Jun 13, 2020

burawi commented Jul 7, 2020

aladdin-add commented Aug 14, 2020

Update: Add exceptionPatterns to id-length rule (fixes #13094) #13099

Update: Add exceptionPatterns to id-length rule (fixes #13094) #13099

Conversation

burawi commented Mar 26, 2020

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

anikethsaha left a comment

Choose a reason for hiding this comment

mdjermanovic commented May 16, 2020

mdjermanovic left a comment

Choose a reason for hiding this comment

mdjermanovic May 21, 2020

Choose a reason for hiding this comment

mdjermanovic May 21, 2020

Choose a reason for hiding this comment

mdjermanovic May 21, 2020

Choose a reason for hiding this comment

mdjermanovic May 21, 2020

Choose a reason for hiding this comment

mdjermanovic May 21, 2020

Choose a reason for hiding this comment

kaicataldo commented Jun 13, 2020

burawi commented Jul 7, 2020

aladdin-add commented Aug 14, 2020