Pass through octal and binary literals as-is #4318
Conversation
|
|
||
| numberValue = number | ||
|
|
||
| unless /^(0o[0-7]+)/.test(number) and /^(0b[01]+)/.test(number) |
There was a problem hiding this comment.
Isn’t this a constant condition (unless false)?
There was a problem hiding this comment.
Good catch, thanks.
| toJS = (str) -> | ||
| CoffeeScript.compile str, bare: yes | ||
| .replace /^\s+|\s+$/g, '' # Trim leading/trailing whitespace | ||
|
|
There was a problem hiding this comment.
Following the style of all other tests (except import/export), all these added tests should be removed.
There was a problem hiding this comment.
Oh, I didn't know that. So no compilation tests at all?
There was a problem hiding this comment.
Yeah, that’s been the rule. Only check that the CoffeeScript does what is expected.
There was a problem hiding this comment.
Ok, cool. 👍 I like removing things.
JimPanic
force-pushed
the
octal-and-binaries-as-is
branch
2 times, most recently
from
499bc0d to
a940a77
Compare
|
Removed the tests and fixed the logic. |
|
@JimPanic While reviewing this PR, I found a bug in master. I fixed that bug, and have also updated the 2 branch with the fix. Could you please rebase this PR? |
|
Just to be clear: is there a reason to parse hex, oct and binary in |
|
Oh, of course they need to be parsed - for the Infinity check. |
|
Yep. Btw, you seem to have completely messed up the commits now :) git is so much fun |
|
Yes, totally. And your fix confused me. :) Basically you refactored it so well, I just have to remove two lines again. So this will be a new force push to my branch; on top of your last commit on 2. |
JimPanic
force-pushed
the
octal-and-binaries-as-is
branch
from
9cfca02 to
329b2d1
Compare
|
Nice! LGTM. @GeoffreyBooth what about you? |
|
Is this really just +1 −2 changes? It doesn’t get much simpler than that. Looks good to me assuming we don’t need any tests (?) |
|
I've missed compiled output tests from time to time. For example, when reviewing this PR, I found an output bug that still gave the same runtime result. It would be nice to write tests for such things. I've just never thought about it before, since I just accepted the style that was already used. Perhaps it's time for a change. |
|
Hence why I added the tests in the first place, but I didn't know there was this rule. We could have compilation tests in a seperate folder/repository maybe? |
|
Compilation tests shouldn't be a blocker for this PR, though? That's a separate issue? |
|
Yeah, that's definitely something for a different issue. |
See coffeescript6/discuss#45
Note: I'm deliberately creating this PR towards branch 2 because it is not opt-in and I'm not sure how many current JS environments implement the binary and octal literals as ES2015 does.