NumberLiteralSeparator: Stage 1 feature plugin. Closes gh-538 #541

rwaldron · 2017-05-26T18:58:40Z

Signed-off-by: Rick Waldron [email protected]

Q	A
Bug fix?	no
Breaking change?	no
New feature?	yes
Deprecations?	no
Spec compliancy?	yes
Tests added/pass?	yes
Fixed tickets	#538
License	MIT

I modified readInt to ignore _ characters found in decimal, hex, binary and octal literals. It will also throw SyntaxError exceptions when the _ is misused.

Signed-off-by: Rick Waldron <[email protected]>

hzoo · 2017-05-26T19:04:27Z

Amazing, that was super quick!

A few things to note:

We need this to be under a flag like the other plugins. This just means creating a hasPlugin('numericSeparators) check so that this functionality isn't turned on by default.
- You can check @diervo's PR as an example of this: Modify grammar to support Private Fields proposal: #260
Update the readme to add that numericSeparators line to the plugins list
Update the ast/spec.md (maybe no update needed)

rwaldron · 2017-05-26T19:05:53Z

@hzoo thanks for the quick review turnaround, I will get these issues addressed and ping you when ready :)

hzoo · 2017-05-26T19:13:45Z

Yeah, I should probably add that information to the contributing, let us know if there's anything that can be clarified or is frustrating

Signed-off-by: Rick Waldron <[email protected]>

codecov · 2017-05-26T19:14:30Z

Codecov Report

Merging #541 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
+ Coverage   98.13%   98.13%   +<.01%     
==========================================
  Files          22       22              
  Lines        3588     3599      +11     
  Branches     1002     1007       +5     
==========================================
+ Hits         3521     3532      +11     
  Misses         24       24              
  Partials       43       43

Flag	Coverage Δ
#babel	`80.41% <16.66%> (-0.22%)`	⬇️
#babylon	`96.8% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
src/tokenizer/index.js	`98.4% <100%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6eeb031...465d80b. Read the comment docs.

rwaldron · 2017-05-26T19:15:20Z

We need this to be under a flag like the other plugins. This just means creating a hasPlugin('numericSeparators) check so that this functionality isn't turned on by default.

You can check @diervo's PR as an example of this: Modify grammar to support Private Fields proposal: #260

Update the readme to add that numericSeparators line to the plugins list

[] ~~Update the ast/spec.md (maybe no update needed)~~

@hzoo changes pushed. I didn't make changes to ast/spec.md because I don't believe there are any changes to make there (I could be wrong, but need guidance)

…alSeparatorSiblings Signed-off-by: Rick Waldron <[email protected]>

hzoo · 2017-05-26T19:23:52Z

src/tokenizer/index.js

+          if (forbiddenNumericLiteralSeparatorSiblings.includes(prev) ||
+              forbiddenNumericLiteralSeparatorSiblings.includes(next) ||
+              Number.isNaN(next)) {
+            this.raise(this.state.pos, "Invalid NumericLiteralSeparator");


I might actually create a different error message for multi _, if it's at the end or the beginning, or around invalid chars but we can totally do that later too.

Could be a good 2nd PR

DrewML · 2017-05-26T19:30:01Z

src/tokenizer/index.js

+        const prev = this.input.charCodeAt(this.state.pos - 1);
+        const next = this.input.charCodeAt(this.state.pos + 1);
+        if (code === 95) {
+          if (forbiddenNumericLiteralSeparatorSiblings.includes(prev) ||


Thanks for the PR, @rwaldron! I believe we need to switch to Array#indexOf here instead of Array#includes. We don't load any polyfills in Babylon, and the minimum supported node version is 4.0.0.

Dang forgot about that 🤔

Ah, ok, no problem. I saw all the other modern JS and just assumed I could use it...whoops! Will fix now.

hzoo

Looks good to me! 👍 Amazing work for the first PR

Ideally the cases that have a weird error message should be fixed to have the same error message as the others

✔ ._1_1 is Unexpected token (1:0) (module)
✔ ._1_1 is Unexpected token (1:0) (script)
✔ 0o01_8 is Unexpected token, expected ; (1:5) (module)
✔ 0o01_8 is Unexpected token, expected ; (1:5) (script)
✔ 0b2_1 is Expected number in radix 2 (1:2) (module)
✔ 0b2_1 is Expected number in radix 2 (1:2) (script)
✔ 0xZ_1 is Expected number in radix 16 (1:2) (module)
✔ 0xZ_1 is Expected number in radix 16 (1:2) (script)

Signed-off-by: Rick Waldron <[email protected]>

rwaldron · 2017-05-26T20:16:35Z

@DrewML pushed change for includes -> indexOf

existentialism

👍

rwaldron · 2017-05-26T20:28:01Z

Ideally the cases that have a weird error message should be fixed to have the same error message as the others

I believe most those stay as they are because they have a different violation in them before the illegal _, or are already illegal and I wanted to ensure that the new code didn't erroneously consider them a new kind of restriction.

._ is illegal today for

I think this:

0o01_8 is Unexpected token, expected ; (1:5) (module)

...is an outlier that should be fixed, but I'm not sure how. The _ is legal here, the 8 is not—it should have an error that says Expected number in radix 8. Seems like an unrelated issue for another ticket.

And this:

._1_1 is Unexpected token (1:0) (module)

... involves changing readToken_dot to raise an exception, which I could do here or in a follow up PR. A follow up might be nicer to keep the error message fixes together.

hzoo · 2017-05-26T20:33:18Z

Yeah I think this is good for this PR

hzoo · 2017-05-26T20:37:13Z

Let's do this 😄

jridgewell · 2017-05-26T21:38:38Z

Does _123 pass?

diervo · 2017-05-26T21:41:23Z

It shouldn't the separator is only allowed in between numeric values.

hzoo · 2017-05-27T02:44:10Z

Well _123 starts with _ so it should just be parsed as an Identifier right?

rwaldron · 2017-05-30T18:16:01Z

@jridgewell @hzoo @diervo correct, _123 is not valid DecimalDigits, because _ is a valid IdentifierStart character. I'm sure it's fine, but I will open a PR that adds a test which asserts that this remains true when the numericSeparator plugin is active.

michaelficarra · 2017-07-28T16:49:52Z

@rwaldron I believe there's a bug in that this allows legacy octal integer literals to have separators. Even if this does restrict them, I don't see a test for that. Do you mind sending a PR to correct that?

michaelficarra · 2017-08-03T19:24:36Z

Ping @rwaldron.

NumberLiteralSeparator: Stage 1 feature plugin

e9598bd

Signed-off-by: Rick Waldron <[email protected]>

hzoo added the spec-new label May 26, 2017

fix-up per review

244cade

Signed-off-by: Rick Waldron <[email protected]>

nit: forbiddenNumericLiteralSeparatorSibling -> forbiddenNumericLiter…

d4ae979

…alSeparatorSiblings Signed-off-by: Rick Waldron <[email protected]>

hzoo added the Spec: Numeric Separator label May 26, 2017

hzoo reviewed May 26, 2017

View reviewed changes

DrewML reviewed May 26, 2017

View reviewed changes

hzoo approved these changes May 26, 2017

View reviewed changes

fix-up to change includes -> indexOf

465d80b

Signed-off-by: Rick Waldron <[email protected]>

existentialism approved these changes May 26, 2017

View reviewed changes

hzoo merged commit b344f62 into babel:master May 26, 2017

hzoo mentioned this pull request May 26, 2017

Numeric Separator: Stage 1 babel/babel#5783

Closed

hzoo added the Tag: New Feature label May 27, 2017

littledan mentioned this pull request Jun 6, 2017

BigInt: Stage 2 #569

Closed

hzoo mentioned this pull request Jul 27, 2017

Numeric Separator (Stage 3) babel/proposals#20

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NumberLiteralSeparator: Stage 1 feature plugin. Closes gh-538 #541

NumberLiteralSeparator: Stage 1 feature plugin. Closes gh-538 #541

rwaldron commented May 26, 2017 •

edited by hzoo

Loading

hzoo commented May 26, 2017

rwaldron commented May 26, 2017

hzoo commented May 26, 2017

codecov bot commented May 26, 2017 •

edited

Loading

rwaldron commented May 26, 2017

hzoo May 26, 2017 •

edited

Loading

DrewML May 26, 2017 •

edited

Loading

hzoo May 26, 2017

rwaldron May 26, 2017

hzoo left a comment

rwaldron commented May 26, 2017

existentialism left a comment

rwaldron commented May 26, 2017

hzoo commented May 26, 2017

hzoo commented May 26, 2017

jridgewell commented May 26, 2017

diervo commented May 26, 2017

hzoo commented May 27, 2017

rwaldron commented May 30, 2017

michaelficarra commented Jul 28, 2017

michaelficarra commented Aug 3, 2017

NumberLiteralSeparator: Stage 1 feature plugin. Closes gh-538 #541

NumberLiteralSeparator: Stage 1 feature plugin. Closes gh-538 #541

Conversation

rwaldron commented May 26, 2017 • edited by hzoo Loading

hzoo commented May 26, 2017

rwaldron commented May 26, 2017

hzoo commented May 26, 2017

codecov bot commented May 26, 2017 • edited Loading

Codecov Report

rwaldron commented May 26, 2017

hzoo May 26, 2017 • edited Loading

Choose a reason for hiding this comment

DrewML May 26, 2017 • edited Loading

Choose a reason for hiding this comment

hzoo May 26, 2017

Choose a reason for hiding this comment

rwaldron May 26, 2017

Choose a reason for hiding this comment

hzoo left a comment

Choose a reason for hiding this comment

rwaldron commented May 26, 2017

existentialism left a comment

Choose a reason for hiding this comment

rwaldron commented May 26, 2017

hzoo commented May 26, 2017

hzoo commented May 26, 2017

jridgewell commented May 26, 2017

diervo commented May 26, 2017

hzoo commented May 27, 2017

rwaldron commented May 30, 2017

michaelficarra commented Jul 28, 2017

michaelficarra commented Aug 3, 2017

rwaldron commented May 26, 2017 •

edited by hzoo

Loading

codecov bot commented May 26, 2017 •

edited

Loading

hzoo May 26, 2017 •

edited

Loading

DrewML May 26, 2017 •

edited

Loading