feat: `id-length` counts graphemes instead of code units by sosukesuzuki · Pull Request #16321 · eslint/eslint

sosukesuzuki · 2022-09-17T07:04:38Z

Prerequisites checklist

I have read the contributing guidelines.

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?

netlify · 2022-09-17T07:05:27Z

✅ Deploy Preview for docs-eslint canceled.

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

snitin315 · 2022-09-17T11:40:28Z

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


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.𐌘 } = {});

Thank you! 39928ab

snitin315

LGTM. Thank you!

I'll leave this open for others to review.

nzakas

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

mdjermanovic

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 · 2022-09-22T19:56:53Z

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

sosukesuzuki · 2022-09-23T09:00:24Z

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.

mdjermanovic · 2022-09-24T17:31:43Z

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

nzakas

LGTM. Just waiting for @mdjermanovic

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

mdjermanovic

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

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

mdjermanovic

LGTM, thanks for contributing!

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 ([#16348](eslint/eslint#16348)) (Brandon Mills) - [`6964cb1`](eslint/eslint@6964cb1) feat: remove support for ignore files in FlatESLint ([#16355](eslint/eslint#16355)) (Milos Djermanovic) - [`1cc4b3a`](eslint/eslint@1cc4b3a) feat: `id-length` counts graphemes instead of code units ([#16321](eslint/eslint#16321)) (Sosuke Suzuki) #### Documentation - [`90c6028`](eslint/eslint@90c6028) docs: Conflicting fixes ([#16366](eslint/eslint#16366)) (Ben Perlmutter) - [`5a3fe70`](eslint/eslint@5a3fe70) docs: Add VS to integrations page ([#16381](eslint/eslint#16381)) (Maria José Solano) - [`49bd1e5`](eslint/eslint@49bd1e5) docs: remove unused link definitions ([#16376](eslint/eslint#16376)) (Nick Schonning) - [`3bd380d`](eslint/eslint@3bd380d) docs: typo cleanups for docs ([#16374](eslint/eslint#16374)) (Nick Schonning) - [`b3a0837`](eslint/eslint@b3a0837) docs: remove duplicate words ([#16378](eslint/eslint#16378)) (Nick Schonning) - [`a682562`](eslint/eslint@a682562) docs: add `BigInt` to `new-cap` docs ([#16362](eslint/eslint#16362)) (Sosuke Suzuki) - [`f6d57fb`](eslint/eslint@f6d57fb) docs: Update docs README ([#16352](eslint/eslint#16352)) (Ben Perlmutter) - [`7214347`](eslint/eslint@7214347) docs: fix logical-assignment-operators option typo ([#16346](eslint/eslint#16346)) (Jonathan Wilsson) #### Chores - [`1f78594`](eslint/eslint@1f78594) chore: upgrade [@eslint/eslintrc](https://github.com/eslint/eslintrc)[@1](https://github.com/1).3.3 ([#16397](eslint/eslint#16397)) (Milos Djermanovic) - [`8476a9b`](eslint/eslint@8476a9b) chore: Remove CODEOWNERS ([#16375](eslint/eslint#16375)) (Nick Schonning) - [`720ff75`](eslint/eslint@720ff75) chore: use "ci" for Dependabot commit message ([#16377](eslint/eslint#16377)) (Nick Schonning) - [`42f5479`](eslint/eslint@42f5479) chore: bump actions/stale from 5 to 6 ([#16350](eslint/eslint#16350)) (dependabot\[bot]) - [`e5e9e27`](eslint/eslint@e5e9e27) chore: remove `jsdoc` dev dependency ([#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. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  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>

fix: counts graphemes instead of code units

81deaf6

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

snitin315 reviewed Sep 17, 2022

View reviewed changes

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

Sosuke Suzuki added 2 commits September 18, 2022 00:33

test: add more tests

39928ab

test: add more valid tests

51508d7

snitin315 approved these changes Sep 18, 2022

View reviewed changes

nzakas requested changes Sep 21, 2022

View reviewed changes

mdjermanovic reviewed Sep 22, 2022

View reviewed changes

Sosuke Suzuki added 2 commits September 23, 2022 17:26

docs: mention about grapehemes

04802f9

fix: first check if string is ASCII

a6beb2e

mdjermanovic reviewed Sep 23, 2022

View reviewed changes

Comment thread lib/rules/id-length.js

fix: use regex

61d28d1

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 Bot added the feature This change adds a new feature to ESLint label Sep 24, 2022

nzakas approved these changes Sep 26, 2022

View reviewed changes

mdjermanovic requested changes Sep 27, 2022

View reviewed changes

Comment thread docs/src/rules/id-length.md Outdated

Comment thread tests/lib/rules/id-length.js

Comment thread tests/lib/rules/id-length.js

sosukesuzuki and others added 3 commits September 28, 2022 00:36

Update docs/src/rules/id-length.md

9249132

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

fix: add max: 1 to valid tests

cb84c8c

fix: add tests for Variation Selectors

bc16453

mdjermanovic reviewed Sep 27, 2022

View reviewed changes

Comment thread tests/lib/rules/id-length.js Outdated

Comment thread tests/lib/rules/id-length.js Outdated

sosukesuzuki and others added 2 commits September 28, 2022 01:38

Update tests/lib/rules/id-length.js

cd906c0

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update tests/lib/rules/id-length.js

edab15b

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

mdjermanovic approved these changes Sep 27, 2022

View reviewed changes

mdjermanovic merged commit 1cc4b3a into eslint:main Sep 27, 2022

sosukesuzuki deleted the fix-16316 branch September 27, 2022 16:47

mdjermanovic added the contributor pool label Sep 27, 2022

sosukesuzuki mentioned this pull request Oct 1, 2022

fix(eslint-plugin): [ban-ts-comment] counts graphemes instead of String.prototype.length typescript-eslint/typescript-eslint#5704

Merged

3 tasks

sync-by-unito Bot mentioned this pull request Oct 10, 2022

build(deps-dev): bump eslint from 8.20.0 to 8.25.0 SmartTokenLabs/token-negotiator#420

Merged

moul-bot mentioned this pull request Oct 10, 2022

chore(deps-dev): bump eslint from 7.32.0 to 8.25.0 in /js berty/berty#4459

Closed

eslint-github-bot Bot locked and limited conversation to collaborators Mar 27, 2023

eslint-github-bot Bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 27, 2023

Uh oh!

Uh oh!

Conversation

sosukesuzuki commented Sep 17, 2022

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?

Uh oh!

netlify Bot commented Sep 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for docs-eslint canceled.

Uh oh!

snitin315 Sep 17, 2022

Choose a reason for hiding this comment

Uh oh!

sosukesuzuki Sep 17, 2022

Choose a reason for hiding this comment

Uh oh!

snitin315 left a comment

Choose a reason for hiding this comment

Uh oh!

nzakas left a comment

Choose a reason for hiding this comment

Uh oh!

mdjermanovic left a comment

Choose a reason for hiding this comment

Uh oh!

nzakas commented Sep 22, 2022

Uh oh!

sosukesuzuki commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mdjermanovic commented Sep 24, 2022

Uh oh!

nzakas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdjermanovic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mdjermanovic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify Bot commented Sep 17, 2022 •

edited

Loading

sosukesuzuki commented Sep 23, 2022 •

edited

Loading