Non-ASCII character support by mertcanaltin · Pull Request #45736 · nodejs/node · GitHub

Is the character's ASCII code greater than 127? (In this case, the function returns true)
Is the character a number? (In this case, the function returns false)
Is the character a "#" symbol? (In this case, the function returns false)
If no conditions are met, the function returns true.

test

 const  isLiteralSymbol = (char) => {
    const code = char.charCodeAt(0);

    if (code > 127) {
      return true;
    }

    if (char >= '0' && char <= 9) {
      return false;
    }

    if (code === 35) {
      return false;
    }

    return true;
  }

output

isLiteralSymbol('أهلا')
true

@anonrig I applied the changes, thank you very much for the review 🚀

@manekinekko Can you review this?

the change LGTM, but this seems to break quite a few tests

Could you:

group these into 2 groups: literals (true) and non-literals (false)?
add more samples of non-Latin characters? you can cherry-pick from this list here.

@manekinekko thank you so much i will fly here 🚀

@manekinekko would that be healthy

{
  const literals = ['A', 'a', '-', '+', 'أ', 'ت', 'ث', '讲', '演', '講'];
  const nonLiterals = ['0', '#', '\\', '+', '-'];
  
  literals.forEach((literal) => {
  assert.strictEqual(isLiteralSymbol(literal), true);
  });
  
  nonLiterals.forEach((nonLiteral) => {
  assert.strictEqual(isLiteralSymbol(nonLiteral), false);
  });
}

I'm so sorry I triggered you all to review, sorry for the extra notifications :/

@manekinekko I sent the edits thank you very much

Should we really accept zero width characters as acceptable input? I would skip all of them. We also already have a function to check for these:

node/lib/internal/util/inspect.js

Lines 2388 to 2398 in ab064d1

    
           const isZeroWidthCodePoint = (code) => { 
        
             return code <= 0x1F || // C0 control codes 
        
               (code >= 0x7F && code <= 0x9F) || // C1 control codes 
        
               (code >= 0x300 && code <= 0x36F) || // Combining Diacritical Marks 
        
               (code >= 0x200B && code <= 0x200F) || // Modifying Invisible Characters 
        
               // Combining Diacritical Marks for Symbols 
        
               (code >= 0x20D0 && code <= 0x20FF) || 
        
               (code >= 0xFE00 && code <= 0xFE0F) || // Variation Selectors 
        
               (code >= 0xFE20 && code <= 0xFE2F) || // Combining Half Marks 
        
               (code >= 0xE0100 && code <= 0xE01EF); // Variation Selectors 
        
           };

I didn't accept zero-width characters @BridgeAR i will update the code like this

  if (typeof char !== 'string' || util.inspect.isZeroWidthCodePoint(char)) {
      return false;
    }

can I do it like this?

@mertcanaltin I think that would be ok.

CC @nodejs/test_runner @manekinekko additional reviews will be appreciated

@anonrig can you please dismiss your review/approve?

CI: https://ci.nodejs.org/job/node-test-pull-request/49692/

Landed in 3c6547f

🎉

PR-URL: #45736 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

PR-URL: nodejs#45736 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

PR-URL: #45736 Backport-PR-URL: #46839 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Dec 4, 2022

anonrig requested changes Dec 4, 2022

View reviewed changes

Comment thread lib/internal/test_runner/tap_lexer.js Outdated

Comment thread lib/internal/test_runner/tap_lexer.js Outdated

Comment thread test/parallel/test-runner-tap-lexer.js Outdated

anonrig added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Dec 4, 2022

mertcanaltin requested a review from anonrig December 4, 2022 22:36

anonrig requested changes Dec 4, 2022

View reviewed changes

anonrig requested a review from MoLow December 4, 2022 23:03

MoLow mentioned this pull request Dec 5, 2022

fix: tap parser fails when test name includes non ASCII characters #45741

Closed

MoLow reviewed Dec 5, 2022

View reviewed changes

manekinekko reviewed Dec 5, 2022

View reviewed changes

mertcanaltin requested review from MoLow, anonrig and manekinekko and removed request for MoLow, anonrig and manekinekko December 5, 2022 14:59

manekinekko reviewed Dec 5, 2022

View reviewed changes

Comment thread lib/internal/test_runner/tap_lexer.js Outdated

Comment thread lib/internal/test_runner/tap_lexer.js Outdated

Comment thread test/parallel/test-runner-tap-lexer.js Outdated

mertcanaltin requested a review from manekinekko December 5, 2022 16:58

BridgeAR reviewed Dec 6, 2022

View reviewed changes

mertcanaltin closed this Dec 7, 2022

mertcanaltin reopened this Dec 7, 2022

mertcanaltin force-pushed the bug-45706 branch from 78c70a1 to 4697163 Compare February 8, 2023 08:52

test_runner: parse non-ascii character correctly

eeb6f02

mertcanaltin force-pushed the bug-45706 branch from e05f17f to eeb6f02 Compare February 8, 2023 09:05

MoLow approved these changes Feb 8, 2023

View reviewed changes

MoLow removed the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Feb 8, 2023

anonrig approved these changes Feb 8, 2023

View reviewed changes

MoLow added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Feb 9, 2023

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2023

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 18, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 18, 2023

nodejs-github-bot merged commit 3c6547f into nodejs:main Feb 18, 2023

mertcanaltin changed the title [WIP] Non-ASCII character support Non-ASCII character support Feb 18, 2023

MylesBorins pushed a commit that referenced this pull request Feb 18, 2023

test_runner: parse non-ascii character correctly

c2d735e

PR-URL: #45736 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

MylesBorins mentioned this pull request Feb 19, 2023

v19.7.0 proposal #46725

Merged

MylesBorins pushed a commit that referenced this pull request Feb 20, 2023

test_runner: parse non-ascii character correctly

ddf5002

PR-URL: #45736 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

MoLow added the backport-open-v18.x label Feb 25, 2023

MoLow pushed a commit to MoLow/node that referenced this pull request Feb 25, 2023

test_runner: parse non-ascii character correctly

730f11f

PR-URL: nodejs#45736 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

juanarbol pushed a commit that referenced this pull request Mar 3, 2023

test_runner: parse non-ascii character correctly

8bd3c0f

PR-URL: #45736 Backport-PR-URL: #46839 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

juanarbol mentioned this pull request Mar 3, 2023

V18.15.0 proposal #46920

Merged

juanarbol pushed a commit that referenced this pull request Mar 5, 2023

test_runner: parse non-ascii character correctly

894d711

PR-URL: #45736 Backport-PR-URL: #46839 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

	const isZeroWidthCodePoint = (code) => {
	return code <= 0x1F \|\| // C0 control codes
	(code >= 0x7F && code <= 0x9F) \|\| // C1 control codes
	(code >= 0x300 && code <= 0x36F) \|\| // Combining Diacritical Marks
	(code >= 0x200B && code <= 0x200F) \|\| // Modifying Invisible Characters
	// Combining Diacritical Marks for Symbols
	(code >= 0x20D0 && code <= 0x20FF) \|\|
	(code >= 0xFE00 && code <= 0xFE0F) \|\| // Variation Selectors
	(code >= 0xFE20 && code <= 0xFE2F) \|\| // Combining Half Marks
	(code >= 0xE0100 && code <= 0xE01EF); // Variation Selectors
	};

Non-ASCII character support by mertcanaltin · Pull Request #45736 · nodejs/node · GitHub

Uh oh!

Uh oh!

Non-ASCII character support - #45736

Non-ASCII character support#45736nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom mertcanaltin:bug-45706mertcanaltin/node:bug-45706Copy head branch name to clipboard

Conversation

mertcanaltin commented Dec 4, 2022 • edited by MoLow Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mertcanaltin commented Dec 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mertcanaltin commented Dec 4, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anonrig commented Dec 4, 2022

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

manekinekko Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

mertcanaltin Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

mertcanaltin Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mertcanaltin commented Dec 5, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mertcanaltin commented Dec 5, 2022

Uh oh!

BridgeAR left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mertcanaltin commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Dec 12, 2022

Uh oh!

MoLow commented Feb 8, 2023

Uh oh!

MoLow commented Feb 8, 2023

Uh oh!

nodejs-github-bot commented Feb 18, 2023

Uh oh!

nodejs-github-bot commented Feb 18, 2023

Uh oh!

mertcanaltin commented Feb 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Non-ASCII character support#45736
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
mertcanaltin:bug-45706

mertcanaltin commented Dec 4, 2022 •
edited by MoLow
Loading

mertcanaltin commented Dec 4, 2022 •
edited
Loading

mertcanaltin Dec 5, 2022 •
edited
Loading

BridgeAR left a comment •
edited
Loading

mertcanaltin commented Dec 7, 2022 •
edited
Loading