-
-
Notifications
You must be signed in to change notification settings - Fork 66
number suffix type annotations #513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@tabatkins honestly the most heartbreaking thing about all this to me is that I can't have |
@bgotink does this align with your understanding/experience/what you did in your lib? |
alright, tests added. This is ready for final review. |
annotation as a "suffix", instead of prepending it between `(` and `)`. This | ||
makes it possible to, for example, write `10px`, `10.5%`, `512GiB`, etc., which | ||
are equivalent to `(px)10`, `(%)5`, and `(GiB)512`, respectively. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for readability purposes, I think it's worth mentioning the #
escape hatch here with a reference to the "Explicit Suffix Type Annotation" section, at least when it comes to types like u32
. For example, maybe:
To remove ambiguity, some suffixes must be prefixed with
#
: for example,10.0u8
is invalid, but10.0#u8
is. The full list of rules for invalid suffixes is clarified in the "Explicit Suffix Type Annotation" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dropped a suggestion for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, GitHub collapsed it as "outdated" because they're bad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+ after review of suggested changes
annotation as a "suffix", instead of prepending it between `(` and `)`. This | ||
makes it possible to, for example, write `10px`, `10.5%`, `512GiB`, etc., which | ||
are equivalent to `(px)10`, `(%)5`, and `(GiB)512`, respectively. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dropped a suggestion for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This matches what I implemented apart from one test that appears to be wrong and one mistake on my end where the parser skips certain validations on #
suffixes which makes 123#123
equivalent to ("123")123
which is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
integer
can end on an underscore so this is actually valid and equivalent to (abc)123_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct. this is why we can't start a bare suffix with _
, becuase the syntax would parse differently than intended.
The following tests that previously failed now run successfully:
|
Uggghhhhh. That makes sense. Looks like we’re gonna need to be more specific what order things run in. I have an idea for the grammar. |
Co-authored-by: Tab Atkins Jr. <[email protected]>
Co-authored-by: Tab Atkins Jr. <[email protected]>
Co-authored-by: Tab Atkins Jr. <[email protected]>
Co-authored-by: Tab Atkins Jr. <[email protected]>
Co-authored-by: Tab Atkins Jr. <[email protected]>
Looking at this... These should be dropped because they're ok now:
These tests should stay and continue to fail. Once you've hit a
|
uggghhhh I understand why those last 3 tests pass now. I want to do something about it, though. I don't like that. I wonder if there's a good way around it. |
@bgotink @tabatkins I've... kind of changed the rules a bit. They're simpler now. And most importantly: we have I checked #510 and I didn't see any discussion about us tackling these rules, because we were focusing so much on what the rules for the suffix should be that we didn't take a step back and think of these numbers as a whole, and how the number syntax in general could be addressed. Please lmk what you think. I think with these changes, we'll get the results I expected in #513 (comment) |
Some key changes:
|
I'm also wondering: could we just drop the exponent restriction (but keep the |
I don't think the specification prose as currently written does require or even allow a trailing I think either the description of the grammatical language needs tightening, or the grammar may now allow some undesirable constructions with underscores and suffixes. Specifically, I'm not sure whether Consider I'm not sure whether I am misreading the description of the grammar language. If there is an issue, either banning Something shaped like If nothing else, test cases including both underscores within the number and invalid suffixes will be good to ensure that the incorrect readings are detected. I also wonder in a similar way about |
Fixes: #510