search

tc39 / proposal-numeric-separator

A proposal to add numeric literal separators in JavaScript.
https://tc39.es/proposal-numeric-separator
332 stars 28 forks source link

Merging proposal contents? #4

Closed tdd closed 7 years ago

tdd commented 7 years ago

Hey @samuelgoto!

This proposal got brought to my attention by a friend; I see you've had the good fortune of working with Domenic and others on it, so it seems like it's on a good track for TC39 consideration… 😉

I'd been thinking about this for years and recently got around to putting together my own proposal, which lacks many contents of yours but also contains a few things yours doesn't yet (e.g. suggested formal grammar changes); perhaps you'd be interested in looking at it and blending what seems useful into your own?

I'd then "deprecate" my own proposal and redirect people to yours, and if/when applicable would gladly help where I can.

Looking forward to hearing from you,

Best,

samuelgoto commented 7 years ago

Hey Christophe!

Really great to find others thinking along the same lines!! Lets join forces and make this a joint proposal: seems like we are both interested in solving the same problem!!!

I'm trying to do a mental diff here between the texts and I found some very interesting aspects in your proposal. Want to take a stab at forming a common/shared opinion here?

(a) syntax

On Fri, May 19, 2017 at 5:11 AM, Christophe Porteneuve < notifications@github.com> wrote:

Hey Samuel!

This proposal got brought to my attention by a friend; I see you've had the good fortune of working with Domenic and others on it, so it seems like it's on a good track for TC39 consideration… 😉

I'd been thinking about this for years and recently got around to putting together my own proposal https://github.com/tdd/proposal-numeric-underscores, which lacks many contents of yours but also contains a few things yours doesn't yet (e.g. suggested formal grammar changes); perhaps you'd be interested in looking at it and blending what seems useful into your own?

I'd then "deprecate" my own proposal and redirect people to yours, and if/when applicable would gladly help where I can.

Looking forward to hearing from you,

Best,

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/samuelgoto/proposal-numeric-separator/issues/4, or mute the thread https://github.com/notifications/unsubscribe-auth/AAqV6nqu6yI2XU6ome6-L-8BN2459VkVks5r7Yb6gaJpZM4Ngca- .

-- f u cn rd ths u cn b a gd prgmr !

tdd commented 7 years ago

Hey Samuel, great to hear from you!

Restricted vs. loose syntaxes

I am indeed for initial restriction, as are you, which would require further syntax definitions.

However, do note that my proposal, as is, doesn't allow leading underscores (in decimal literals, at least). This is because 11.8.3 defines the DecimalIntegerLiteral production to start with either 0 or a NonZeroDigit production, unaffected by my proposed lexical grammar changes.

Still, as there are no such distinctions for other-radix literals, my proposal as is would indeed allow for leading underscores post-radix prefix, e.g. 0b__01_0000, which we probably don't want. I can try and find some time soon to create an alternate productions change list to address this.

Broadening semantics of parseInt and parseFloat

My proposal introduces backwards-incompatible changes as these APIs, which would in the past stop half in their tracks when encountering an underscore, now deal with it. So parseInt('100_000) would have returned 100, and now returns 100K.

I was, mistakenly, under the impression that ES2015 introduced similar broadening by allowing these APIs to parse '0b101010 or 0o644, which it does not. As TC39 refuses to introduce backwards-incompatible changes to avoid "breaking the web", this makes sense.

In that spirit, I do believe we should explicitly state that parseInt and parseFloat remain unaffected and keep parsing only decimals and hexadecimals, in their traditional forms.

As for merging, please, go ahead! Once done I'll adjust my own proposal to redirect to yours.

Best,

rwaldron commented 7 years ago

TC39 chose to restrict separators to single character, and disallowed separator to appear:

The grammar that achieves this is merged into the draft proposal now

samuelgoto commented 7 years ago

Just to report back on this thread here and try to merge the proposal. I brought this to TC39 and we got to stage 1 with the guidance to keep parseFlow backwards compatible but break the Number constructor.

As @rwaldron mentioned, we ended up with the following proposal for the grammar.

Your cross-cutting concerns section is richer than mine, so let me try to merge that first (feel free to send a PR for other sections if you'd like, e.g. better examples, better motivation, etc).

Generally, the grammar proposed doesn't change the definition of the digits, so the algorithms in 7.1.3.1 for ToNumber/ToXX remain entirely unchanged/untouched. Questions:

(a) Does that change any of your analysis on the isFinite, isNan and ToXX methods? (b) Number() does need change, I think, to accommodate to the ability to parse the _s.

If you send these answers as a PR we can move this discussion to the PR and work from there, either way works for me.

rwaldron commented 7 years ago

@samuelgoto lol, I've had a PR open for about a week ;)

samuelgoto commented 7 years ago

I know I'm sorry I was OOO :) will get to it super quickly today I promise!! Sorry :(

(sent from phone, apologies for the brevity)

On Jun 6, 2017 11:51 AM, "Rick Waldron" notifications@github.com wrote:

@samuelgoto https://github.com/samuelgoto lol, I've had a PR open for about a week ;)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/samuelgoto/proposal-numeric-separator/issues/4#issuecomment-306582029, or mute the thread https://github.com/notifications/unsubscribe-auth/AAqV6hy7H0FadczpNZwXdLKlYh-Dpvn0ks5sBZ-XgaJpZM4Ngca- .

tdd commented 7 years ago

Hey guys,

Delighted to see this moving forward. I had been following the issues and PR's here and saw it got accepted to stage 1 🎉 — well done Samuel on presenting it at the latest TC39 meeting! Saw your slides, appreciated the hat tip btw.

(Also: hey, Rick! It's been a while 😉)

As for your questions:

(a) As everything you mention is ToNumber-based, and the latter relies on digit defs which the current proposed grammar doesn't alter, I think we're safe as-is, yes. We should also leave parseInt and parseFloat untouched (they already don't recognize the new octal/binary syntaxes). (b) Number(…) indeed needs to change, and if I read other discussions, and feedback from Rick, properly, TC39 encourages adjusting it. Its current definition relies entirely on ToNumber, so it won't be able to recognize the separator as-is. I'm unsure how to best alter the spec text, though.

rwaldron commented 7 years ago

@tdd ❤️ ❤️

Its current definition relies entirely on ToNumber, so it won't be able to recognize the separator as-is.

I checked this, and it does :) Follow me: 

StrNumericLiteral:::
  StrDecimalLiteral
  BinaryIntegerLiteral
  OctalIntegerLiteral
  HexIntegerLiteral

StrDecimalLiteral:::
  StrUnsignedDecimalLiteral
  +StrUnsignedDecimalLiteral
  -StrUnsignedDecimalLiteral

StrUnsignedDecimalLiteral:::
  Infinity
  DecimalDigits . DecimalDigits_opt ExponentPart_opt
  . DecimalDigitsExponentPart_opt
  DecimalDigitsExponentPart_opt

...Which all include NumericLiteralSeparator

samuelgoto commented 7 years ago

that's pretty neat @rwaldron ! that makes sense to me too and matches my interpretation too!

i'm a little slow to understand parseInt() though. If I read this correctly, is parseInt entirely indepedent from any of the grammar changes that we are proposing?

what's getting me confused is that @tdd seems to imply that parseInt relies on ToInt32 which does rely on ToNumber which we do change.

what am i missing here?

samuelgoto commented 7 years ago

parseFloat() makes a more explicit reference to StrDecimalLiteral which we do also change, so that means that this would change parseFloat too (which isn't what we want).

do I read that correctly?

tdd commented 7 years ago

Dang it. Yes, you read that correctly. Any change on DecimalDigits will ripple through to StrDecimalLiteral, impacting parseFloat (not parseInt, btw). Exactly the kind of hairy edge-cases we're supposed to weed out at this stage 😉

rwaldron commented 7 years ago

do I read that correctly?

Exactly the kind of hairy edge-cases we're supposed to weed out at this stage

Yes. Not sure of the right solution yet...

rwaldron commented 7 years ago

Here we go:

StrDecimalLiteral:::
  StrUnsignedDecimalLiteral
  + StrUnsignedDecimalLiteral
  - StrUnsignedDecimalLiteral

StrUnsignedDecimalLiteral:::
  Infinity
  StrDecimalDigits . StrDecimalDigits_opt ExponentPart_opt 
  . StrDecimalDigits ExponentPart_opt 
  StrDecimalDigits ExponentPart_opt 

StrDecimalDigits ::
  DecimalDigit
  StrDecimalDigits DecimalDigit
tdd commented 7 years ago

Hey @rwaldron

(for some reason despite my following this repo and sub'ing to this thread, I didn't see the new comments popping up in my inbox 😒 …)

Hmmmm I'll have to check the grammar with these changes. The intent here is to make sure parseFloat doesn't get impacted by the new syntax and keeps halting on underscores, right?

rwaldron commented 7 years ago

@tdd yep, that's exactly the intention. Here's a diff: 

  StrDecimalLiteral:::
    StrUnsignedDecimalLiteral
    + StrUnsignedDecimalLiteral
    - StrUnsignedDecimalLiteral

  StrUnsignedDecimalLiteral:::
    Infinity
-   DecimalDigits . DecimalDigits_opt ExponentPart_opt 
-   . DecimalDigits ExponentPart_opt 
-   DecimalDigits ExponentPart_opt 
+   StrDecimalDigits . StrDecimalDigits_opt ExponentPart_opt 
+   . StrDecimalDigits ExponentPart_opt 
+   StrDecimalDigits ExponentPart_opt 

+ StrDecimalDigits ::
+   DecimalDigit
+   StrDecimalDigits DecimalDigit

All it does is ensure that parseFloat's rules don't include the newly updated DecimalDigits, by making StrDecimalDigits as a stand-in for DecimalDigits

tdd commented 7 years ago

Oooooh that's sweet. I like it!

tdd commented 7 years ago

@samuelgoto unrelated: have you had a moment to prep a PR for merging the value-added parts of my own proposal in this one's README (you liked the cross-cutting concerns, etc.)?

If not, I could give it a go. I'm just pretty tight on schedule right now so wouldn't want to duplicate an ongoing effort.

rwaldron commented 7 years ago

@tdd I wrote a PR for merging the content: https://github.com/tc39/proposal-numeric-separator/pull/15

I've also created a separate PR for updating the spec: https://github.com/tc39/proposal-numeric-separator/pull/16

samuelgoto commented 7 years ago

Ha interesting trick! A little bit of duplication but seems like a necessary one. Looking at the PRs now.

FWIW we moved to the tc39 org repo now so I'm assuming most development gets done there.

Sam

(sent from phone, apologies for the brevity)

On Jun 14, 2017 7:55 AM, "Rick Waldron" notifications@github.com wrote:

@tdd https://github.com/tdd I wrote a PR for merging the content: #15 https://github.com/tc39/proposal-numeric-separator/pull/15

I've also created a separate PR for updating the spec: #16 https://github.com/tc39/proposal-numeric-separator/pull/16

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tc39/proposal-numeric-separator/issues/4#issuecomment-308457470, or mute the thread https://github.com/notifications/unsubscribe-auth/AAqV6rFuGs_h4G8i6pN0OFIWItRvk0qmks5sD_RtgaJpZM4Ngca- .

samuelgoto commented 7 years ago

I just looked at the last PR now and I merged things into the master branch. Thanks @tdd and @rwaldron for the PR!! Closing.