Numbers should be allowed to contain underscores. #1844
Conversation
… plus tests in s/src/javascript/tokenizer.js and test/data/javascript/tests.js
… plus tests in s/src/javascript/tokenizer.js and test/data/javascript/tests.js modified: js/src/javascript/tokenizer.js modified: js/test/generated/beautify-javascript-tests.js modified: python/jsbeautifier/tests/generated/tests.py modified: test/data/javascript/tests.js
modified: python/jsbeautifier/javascript/tokenizer.py modified: test/data/javascript/tests.js Issue beautifier#1836 updated number_pattern regex to include underscore in binary literal.
…js-beautify into issue/underscores_1836 Merging earlier commit to my local branch no changes made. Rerunning tests.
modified: test/data/javascript/tests.js Test suite made change to tests.js file due to style issue.
mrmonroe
changed the title
|
Hey guys, The other failures are from the ./tools/git-status-clear.sh script running in the Azure pipeline which looks like an outstanding issue, although I don't see it logged. I ran the script on my Mac and Linux boxes and have no problems with it. I'd be glad to help you guys with it but of course, I don't have access to the Azure account. Let me know how you want to proceed. |
Please feel free to break the regex to multiple lines/variables for clarity. Thanks for the heads up on the CI failure, I'll look into that. |
There was a problem hiding this comment.
Please add tests as noted.
Also, the current change only addresses 0b numbers. Will you be updating to handle all number forms? If so, please include tests for similar scenarios to the ones noted for other numbers.
Keep in mind _ is the start of a variable name, so ._001 cannot be treated as a number during tokenizing. It will need to be treated as . and a variable _001.
There's a bunch of edge cases.
Thanks for contributing this PR!
js/src/javascript/tokenizer.js
Outdated
| @@ -66,7 +66,7 @@ var TOKEN = { | |||
|
|
|||
| var directives_core = new Directives(/\/\*/, /\*\//); | |||
|
|
|||
| var number_pattern = /0[xX][0123456789abcdefABCDEF]*|0[oO][01234567]*|0[bB][01]*|\d+n|(?:\.\d+|\d+\.?\d*)(?:[eE][+-]?\d+)?/; | |||
| var number_pattern = /0[xX][0123456789abcdefABCDEF]*|0[oO][01234567]*|0[bB][01_]*|\d+n|(?:\.\d+|\d+\.?\d*)(?:[eE][+-]?\d+)?/; | |||
There was a problem hiding this comment.
Numbers still have to start with a number. 0b_101 is not valid, 0b1_01 is valid. Also, 0b1__1 is also not valid syntax either.
Note, 0b is not valid and the current regex will match it, and the result can be seen here: bt('a=0Bg0b0o0', 'a = 0B g0b0o0');
The question is whether the beautifier should "fix" problems with underscores the it does with other characters, or if it should be more permissive. :sad: g is never a valid character, but underscore is.
I guess for 0x, 0o, and 0b, the regex you have above is fine, but please add tests for 0b_101 and 0b1__01 so that the behavior doesn't change unexpectedly later.
|
@bitwiseman been down with Covid for a long time and just getting my life back together. Will get to this PR in a few days. |
|
@mrmonroe |
We chose to be more permissive with underscore and not reformate invalid placements of them. Leaving unchanged is slightly odd but it also prevents the beautifier from trying to guess what the user actually meant.
bitwiseman
changed the title
Description
master)Fixes Issue:
#1836 Numbers should be allowed to contain underscores.
Before Merge Checklist
These items can be completed after PR is created.
(Check any items that are not applicable (NA) for this PR)