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.

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

feat: id-length counts graphemes instead of code units #16321

Merged
merged 11 commits into from Sep 27, 2022

Conversation

sosukesuzuki
Copy link
Contributor

Prerequisites checklist

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

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

What changes did you make? (Give an overview)

Fixes #16316

Counts graphemes instead of code units with grapheme-splitter

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

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon bug ESLint is working incorrectly labels Sep 17, 2022
@netlify
Copy link

netlify bot commented Sep 17, 2022

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit edab15b
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6333270c26227b0008b2da85

code: "var 𠮟 = 2",
parserOptions: { ecmaVersion: 6 },
errors: [
tooShortError
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we cover more test cases here? Playground Link

var myObj = { 𐌘: 1 };

(𐌘) => { 𐌘 * 𐌘 };

class 𠮟 { }

class Foo { 𐌘() {} }

class Foo1 { #𐌘() {} }

class Foo2 { 𐌘 = 1 }

class Foo3 { #𐌘 = 1 }

function foo1(...𐌘) { }

function foo([𐌘]) { }

var [ 𐌘 ] = arr;

var { prop: [𐌘]} = {};

function foo({𐌘}) { }

var { 𐌘 } = {};

var { prop: 𐌘} = {};

({ prop: obj.𐌘 } = {});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! 39928ab

@snitin315 snitin315 added rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Sep 17, 2022
Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

I'll leave this open for others to review.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

The code looks good to me. Can you please also update the documentation to explain that this rule counts graphemes instead of characters?

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

The code looks good, but counting graphemes instead of code points / code units has a significant performance impact.

I enabled "id-length": "error" in our config, and ran eslint . with TIMING=all.

Before this change:

Rule                                          | Time (ms) | Relative
:---------------------------------------------|----------:|--------:
indent                                        |  5759.606 |    16.8%
jsdoc/check-access                            |  1141.858 |     3.3%
jsdoc/valid-types                             |  1065.092 |     3.1%
n/no-missing-require                          |   963.300 |     2.8%
n/no-extraneous-require                       |   942.378 |     2.8%
jsdoc/check-types                             |   892.590 |     2.6%
jsdoc/check-tag-names                         |   756.448 |     2.2%
n/no-unpublished-require                      |   710.689 |     2.1%
jsdoc/check-values                            |   660.609 |     1.9%
jsdoc/check-alignment                         |   637.052 |     1.9%
...
id-length                                     |    86.412 |     0.3%

After this change:

Rule                                          | Time (ms) | Relative
:---------------------------------------------|----------:|--------:
indent                                        |  5713.065 |    15.4%
id-length                                     |  3292.387 |     8.9%
jsdoc/check-access                            |  1136.082 |     3.1%
jsdoc/valid-types                             |  1037.168 |     2.8%
n/no-missing-require                          |   936.313 |     2.5%
n/no-extraneous-require                       |   920.256 |     2.5%
jsdoc/check-types                             |   843.173 |     2.3%
jsdoc/check-tag-names                         |   724.359 |     2.0%
n/no-unpublished-require                      |   706.890 |     1.9%
jsdoc/check-values                            |   631.021 |     1.7%

Maybe we should consider making this behavior optional?

@nzakas
Copy link
Member

nzakas commented Sep 22, 2022

Nice catch! Maybe we could first check to see if there are actually any non-ASCII characters before applying the new behavior?

@sosukesuzuki
Copy link
Contributor Author

sosukesuzuki commented Sep 23, 2022

Thanks for your comments about the performance! As @nzakas commented, I did an ASCII check first and used String length if it was ASCII. a6beb2e

The resulting performance was as follows:

Before this PR:

Rule                                          | Time (ms) | Relative
:---------------------------------------------|----------:|--------:
id-length                                     |    81.275 |     0.3%

After this PR:

Rule                                          | Time (ms) | Relative
:---------------------------------------------|----------:|--------:
id-length                                     |   145.700 |     0.5%

If this performance decrease is unacceptable, I think we should add an option.

lib/rules/id-length.js Outdated Show resolved Hide resolved
@mdjermanovic mdjermanovic changed the title fix: id-length counts graphemes instead of code units feat: id-length counts graphemes instead of code units Sep 24, 2022
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Sep 24, 2022
@mdjermanovic
Copy link
Member

Marked as feat because the change in counting can produce more warnings with the same configuration for this rule.

nzakas
nzakas approved these changes Sep 26, 2022
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Just waiting for @mdjermanovic

docs/src/rules/id-length.md Outdated Show resolved Hide resolved
tests/lib/rules/id-length.js Outdated Show resolved Hide resolved
tests/lib/rules/id-length.js Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

A small suggestion to clarify the last two tests, then LGTM

tests/lib/rules/id-length.js Outdated Show resolved Hide resolved
tests/lib/rules/id-length.js Outdated Show resolved Hide resolved
sosukesuzuki and others added 2 commits September 28, 2022 01:38
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contributing!

@mdjermanovic mdjermanovic merged commit 1cc4b3a into eslint:main Sep 27, 2022
20 checks passed
@sosukesuzuki sosukesuzuki deleted the fix-16316 branch September 27, 2022 16:47
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Oct 13, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.24.0` -> `8.25.0`](https://renovatebot.com/diffs/npm/eslint/8.24.0/8.25.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.25.0`](https://github.com/eslint/eslint/releases/tag/v8.25.0)

[Compare Source](eslint/eslint@v8.24.0...v8.25.0)

#### Features

-   [`173e820`](eslint/eslint@173e820) feat: Pass --max-warnings value to formatters ([#&#8203;16348](eslint/eslint#16348)) (Brandon Mills)
-   [`6964cb1`](eslint/eslint@6964cb1) feat: remove support for ignore files in FlatESLint ([#&#8203;16355](eslint/eslint#16355)) (Milos Djermanovic)
-   [`1cc4b3a`](eslint/eslint@1cc4b3a) feat: `id-length` counts graphemes instead of code units ([#&#8203;16321](eslint/eslint#16321)) (Sosuke Suzuki)

#### Documentation

-   [`90c6028`](eslint/eslint@90c6028) docs: Conflicting fixes ([#&#8203;16366](eslint/eslint#16366)) (Ben Perlmutter)
-   [`5a3fe70`](eslint/eslint@5a3fe70) docs: Add VS to integrations page ([#&#8203;16381](eslint/eslint#16381)) (Maria José Solano)
-   [`49bd1e5`](eslint/eslint@49bd1e5) docs: remove unused link definitions ([#&#8203;16376](eslint/eslint#16376)) (Nick Schonning)
-   [`3bd380d`](eslint/eslint@3bd380d) docs: typo cleanups for docs ([#&#8203;16374](eslint/eslint#16374)) (Nick Schonning)
-   [`b3a0837`](eslint/eslint@b3a0837) docs: remove duplicate words ([#&#8203;16378](eslint/eslint#16378)) (Nick Schonning)
-   [`a682562`](eslint/eslint@a682562) docs: add `BigInt` to `new-cap` docs ([#&#8203;16362](eslint/eslint#16362)) (Sosuke Suzuki)
-   [`f6d57fb`](eslint/eslint@f6d57fb) docs: Update docs README ([#&#8203;16352](eslint/eslint#16352)) (Ben Perlmutter)
-   [`7214347`](eslint/eslint@7214347) docs: fix logical-assignment-operators option typo ([#&#8203;16346](eslint/eslint#16346)) (Jonathan Wilsson)

#### Chores

-   [`1f78594`](eslint/eslint@1f78594) chore: upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;1](https://github.com/1).3.3 ([#&#8203;16397](eslint/eslint#16397)) (Milos Djermanovic)
-   [`8476a9b`](eslint/eslint@8476a9b) chore: Remove CODEOWNERS ([#&#8203;16375](eslint/eslint#16375)) (Nick Schonning)
-   [`720ff75`](eslint/eslint@720ff75) chore: use "ci" for Dependabot commit message ([#&#8203;16377](eslint/eslint#16377)) (Nick Schonning)
-   [`42f5479`](eslint/eslint@42f5479) chore: bump actions/stale from 5 to 6 ([#&#8203;16350](eslint/eslint#16350)) (dependabot\[bot])
-   [`e5e9e27`](eslint/eslint@e5e9e27) chore: remove `jsdoc` dev dependency ([#&#8203;16344](eslint/eslint#16344)) (Milos Djermanovic)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

 **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMjEuMSIsInVwZGF0ZWRJblZlciI6IjMyLjIyNi4wIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1577
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly contributor pool feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: id-length doesn't throw error for characters consisting of two code units
4 participants