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.

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

Naming conventions #861

Merged
merged 21 commits into from
Jan 27, 2022
Merged

Naming conventions #861

merged 21 commits into from
Jan 27, 2022

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Sep 29, 2021

  • For idiomatic Carbon code:
    • UpperCamelCase will be used when the identifier can be resolved to a
      specific value at compile-time.
    • lower_snake_case will be used when the identifier's value won't be
      known until runtime, such as for variables.
  • For Carbon-provided features:
    • Keywords and type literals will use lower_snake_case.
    • Other code will use the guidelines for idiomatic Carbon code.

@jonmeow jonmeow added the proposal A proposal label Sep 29, 2021
@jonmeow jonmeow requested a review from a team September 29, 2021 20:09
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Sep 29, 2021
@jonmeow jonmeow marked this pull request as ready for review September 30, 2021 19:43
@github-actions github-actions bot added the proposal rfc Proposal with request-for-comment sent out label Sep 30, 2021
@jonmeow
Copy link
Contributor Author

jonmeow commented Sep 30, 2021

@zygoloid @chandlerc Note, I'm trying to interpret the current rationale for the naming decision. I think I've accurately captured your perspective now: I had definitely misunderstood some of the discussions, but a quick thread with Chandler hopefully cleared that up. Given I may be inserting the wrong words into your mouths, please let me know if I'm still wrong.

proposals/p0861.md Outdated Show resolved Hide resolved
proposals/p0861.md Outdated Show resolved Hide resolved
proposals/p0861.md Outdated Show resolved Hide resolved
proposals/p0861.md Outdated Show resolved Hide resolved
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.

Addressing comments

proposals/p0861.md Outdated Show resolved Hide resolved
proposals/p0861.md Outdated Show resolved Hide resolved
proposals/p0861.md Outdated Show resolved Hide resolved
proposals/p0861.md Outdated Show resolved Hide resolved
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.

Trying to get back to this, again sorry for the delay. Suggestions and comments inline, and thanks for working on pulling all the different discussions together into a proposal.

Comment on lines 81 to 82
- `UpperCamelCase` will be used when the identifier can be resolved to a
specific value at compile-time.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, would "value or entity" wore better or worse here?

Suggested change
- `UpperCamelCase` will be used when the identifier can be resolved to a
specific value at compile-time.
- `UpperCamelCase` will be used when the identifier can be resolved to a
specific value or entity at compile-time.

Specifically thinking about namespaces. @zygoloid might have thoughts on the specific wording of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative way to phrase this we could consider: "UpperCamelCase will be used when the identifier names a specific program entity, such as a function, a namespace, or a compile-time constant value." This tries to anchor more on the "proper noun" kind of rationale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the second suggestion, since I assume you prefer it.

Copy link
Contributor

Choose a reason for hiding this comment

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

One phrasing that came up in discussion with @chandlerc: "if the named entity cannot have a dynamically varying value".

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I think I prefer the latest version ("... names a specific program entity, such as ....") slightly over "cannot have a dynamically varying value". Maybe they could be combined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried rephrasing...

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy with the phrasing that you ended up with. I feel like I could apply it to understand why function parameters get one result and template/generic parameters get another result, and why a local let a: i32 binding gets one result but a global let A: i32 binding gets a different result.

proposals/p0861.md Show resolved Hide resolved
proposals/p0861.md Outdated Show resolved Hide resolved
proposals/p0861.md Outdated Show resolved Hide resolved
proposals/p0861.md Outdated Show resolved Hide resolved
Comment on lines 147 to 148
Private member variables should use a `_` suffix in order to prevent name
collisions. We discourage private properties.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we skip this suggestion, at least for now? I'm not sure it will be needed in Carbon, and I'd rather not get ahead of that design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to an open question; I would emphasize, after #720 #898 #970, I'd really like to avoid digging the same hole in early Carbon code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just like to avoid pre-adopting a workaround for an avoidable design problem... Thanks for moving this to an open question, would it be OK to remove it from the list above as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, but I'm a bit confused by your request... All I see in the above list is properties with no mention of _, so I assume you mean to remove properties altogether -- gone.

proposals/p0861.md Outdated Show resolved Hide resolved
proposals/p0861.md Outdated Show resolved Hide resolved
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.

Addressing comments

Comment on lines 81 to 82
- `UpperCamelCase` will be used when the identifier can be resolved to a
specific value at compile-time.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the second suggestion, since I assume you prefer it.

proposals/p0861.md Show resolved Hide resolved
Comment on lines 147 to 148
Private member variables should use a `_` suffix in order to prevent name
collisions. We discourage private properties.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to an open question; I would emphasize, after #720 #898 #970, I'd really like to avoid digging the same hole in early Carbon code.

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.

I think the only main thing left is making sure we've converged on the least confusing phrasing for the UpperCase stuff. Some minor things elsewhere....

proposals/p0861.md Outdated Show resolved Hide resolved
Comment on lines 81 to 82
- `UpperCamelCase` will be used when the identifier can be resolved to a
specific value at compile-time.
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I think I prefer the latest version ("... names a specific program entity, such as ....") slightly over "cannot have a dynamically varying value". Maybe they could be combined?

Comment on lines 147 to 148
Private member variables should use a `_` suffix in order to prevent name
collisions. We discourage private properties.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just like to avoid pre-adopting a workaround for an avoidable design problem... Thanks for moving this to an open question, would it be OK to remove it from the list above as well?

proposals/p0861.md Outdated Show resolved Hide resolved
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.

Addressing comments

Comment on lines 147 to 148
Private member variables should use a `_` suffix in order to prevent name
collisions. We discourage private properties.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, but I'm a bit confused by your request... All I see in the above list is properties with no mention of _, so I assume you mean to remove properties altogether -- gone.

Comment on lines 81 to 82
- `UpperCamelCase` will be used when the identifier can be resolved to a
specific value at compile-time.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried rephrasing...

proposals/p0861.md Outdated Show resolved Hide resolved
proposals/p0861.md Outdated Show resolved Hide resolved
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.

LGTM with a minor tweak to Base below!

proposals/p0861.md Outdated Show resolved Hide resolved
proposals/p0861.md Outdated Show resolved Hide resolved
proposals/p0861.md Outdated Show resolved Hide resolved
proposals/p0861.md Outdated Show resolved Hide resolved
@jonmeow jonmeow merged commit 93a5fcd into carbon-language:trunk Jan 27, 2022
@jonmeow jonmeow deleted the proposal-naming-conventions branch January 27, 2022 19:25
@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Jan 27, 2022
jonmeow added a commit that referenced this pull request Jan 29, 2022
A pretty straight copy of #861. Cutting down README.md example text since it seemed a little redundant with examples in the bullets.
chandlerc added a commit that referenced this pull request Jun 28, 2022
-   For idiomatic Carbon code:
    -   `UpperCamelCase` will be used when the identifier can be resolved to a
        specific value at compile-time.
    -   `lower_snake_case` will be used when the identifier's value won't be
        known until runtime, such as for variables.
-   For Carbon-provided features:
    -   Keywords and type literals will use `lower_snake_case`.
    -   Other code will use the guidelines for idiomatic Carbon code.

Co-authored-by: Chandler Carruth <[email protected]>
Co-authored-by: Richard Smith <[email protected]>
Co-authored-by: josh11b <[email protected]>
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
A pretty straight copy of #861. Cutting down README.md example text since it seemed a little redundant with examples in the bullets.
ooxi added a commit to ooxi/carbon-lang that referenced this pull request Jul 24, 2022
Primitive types have been renamed in pull request carbon-language#861 and also the design document.
ooxi added a commit to ooxi/carbon-lang that referenced this pull request Jul 24, 2022
Primitive types have been renamed in pull request carbon-language#861 and also the design document.
jonmeow pushed a commit that referenced this pull request Jul 24, 2022
Primitive types have been renamed in pull request #861 and also the design document.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot. proposal accepted Decision made, proposal accepted proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants