Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Weaken digit separator placement rules #1983

Merged
merged 13 commits into from
Aug 25, 2022
Merged

Weaken digit separator placement rules #1983

merged 13 commits into from
Aug 25, 2022

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Aug 10, 2022 •
edited by josh11b
Loading

Proposal #143: Numeric literals added digit separators with strict rules for placement. It missed some use-cases. In order to address this, remove placement rules for numeric literals.

Related issue: #1485

@jonmeow jonmeow added the proposal A proposal label Aug 10, 2022
@jonmeow jonmeow requested a review from a team August 10, 2022 22:20
@jonmeow jonmeow added this to Draft in Proposals via automation Aug 11, 2022
@jonmeow jonmeow marked this pull request as ready for review August 11, 2022 17:57
@jonmeow jonmeow moved this from Draft to RFC in Proposals Aug 11, 2022
@github-actions github-actions bot added the proposal rfc Proposal with request-for-comment sent out label Aug 11, 2022
@jonmeow jonmeow changed the title Digit separators Weaken digit separator placement rules Aug 11, 2022
@jonmeow jonmeow mentioned this pull request Aug 11, 2022
Closed
@josh11b
Copy link
Contributor

josh11b commented Aug 11, 2022 •
edited
Loading

Would it be possible to update https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/lexical_conventions/numeric_literals.md#digit-separators and https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/README.md#integer-literals as part of this PR or file an issue to make those changes?

josh11b
josh11b reviewed Aug 11, 2022
View reviewed changes
proposals/p1983.md Outdated
- For hexadecimal in particular, there are microformats where different
placement rules may be desirable. For example, MAC addresses are typically
in byte pairs (`00-B0-D0-63-C2-26`) and UUIDs have particular groupings
(`123e4567-e89b-12d3-a456-426614174000`).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also decimal numbers: credit card numbers, ISBN, etc.

Copy link
Contributor Author

@jonmeow jonmeow Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed this to point at the alternatives for examples, because that already covers more. (I'd like to avoid duplicating that much content)

@jonmeow jonmeow added proposal draft Proposal in draft, not ready for review and removed proposal draft Proposal in draft, not ready for review labels Aug 12, 2022
@github-actions github-actions bot removed the proposal rfc Proposal with request-for-comment sent out label Aug 12, 2022
@jonmeow jonmeow added the proposal rfc Proposal with request-for-comment sent out label Aug 12, 2022
@github-actions github-actions bot removed the proposal draft Proposal in draft, not ready for review label Aug 12, 2022
chandlerc
chandlerc reviewed Aug 12, 2022
View reviewed changes
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly some minor wording comments on the proposal.

proposals/p1983.md Outdated Show resolved Hide resolved
proposals/p1983.md Show resolved Hide resolved
proposals/p1983.md Outdated Show resolved Hide resolved
@jonmeow @chandlerc
Update proposals/p1983.md
fd7c1e6 
Co-authored-by: Chandler Carruth <[email protected]>
jonmeow
jonmeow commented Aug 12, 2022
View reviewed changes
Copy link
Contributor Author

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Response round

proposals/p1983.md Outdated Show resolved Hide resolved
proposals/p1983.md Show resolved Hide resolved
@jonmeow
Copy link
Contributor Author

jonmeow commented Aug 12, 2022

Would it be possible to update https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/lexical_conventions/numeric_literals.md#digit-separators and https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/README.md#integer-literals as part of this PR or file an issue to make those changes?

I'd prefer to handle updates separately. I don't think we should file an issue for implementing unapproved proposals.

zygoloid
zygoloid reviewed Aug 25, 2022
View reviewed changes
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

proposals/p1983.md Outdated Show resolved Hide resolved
proposals/p1983.md Outdated Show resolved Hide resolved
jonmeow and others added 2 commits August 25, 2022 14:41
@jonmeow @zygoloid
Update proposals/p1983.md
4fa581a 
Co-authored-by: Richard Smith <[email protected]>
@jonmeow @zygoloid
Update proposals/p1983.md
a256ac0 
Co-authored-by: Richard Smith <[email protected]>
zygoloid
zygoloid approved these changes Aug 25, 2022
View reviewed changes
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! The leads are happy with this.

@zygoloid zygoloid merged commit ba2cfc2 into carbon-language:trunk Aug 25, 2022
@jonmeow jonmeow deleted the proposal-digit-separators branch August 25, 2022 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zygoloid zygoloid zygoloid approved these changes

@chandlerc chandlerc Awaiting requested review from chandlerc

@josh11b josh11b Awaiting requested review from josh11b

Assignees
No one assigned
Labels
proposal rfc Proposal with request-for-comment sent out proposal A proposal
Projects
No open projects
Proposals
RFC
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jonmeow @josh11b @zygoloid @chandlerc