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

Allow non-alphabetic underscores in camel case #46907

Merged
merged 4 commits into from Jan 5, 2018

Conversation

Projects
None yet
7 participants
@varkor
Contributor

varkor commented Dec 21, 2017

Certain identifiers, such as X86_64, cannot currently be unambiguously represented in camel case (X8664, X86_64, X8_664, etc. are all transformed to the same identifier). This change relaxes the rules so that underscores are permitted between two non-alphabetic characters under #[forbid(non_camel_case_types)]. Fixes #34633 and fixes #41621.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Dec 21, 2017

Collaborator

r? @michaelwoerister

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Dec 21, 2017

r? @michaelwoerister

(rust_highfive has picked a reviewer for you, use r? to override)

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister
Contributor

michaelwoerister commented Dec 21, 2017

r? @nagisa

@nagisa

This comment has been minimized.

Show comment
Hide comment
@nagisa

nagisa Dec 21, 2017

Contributor

Is it something that needs a dependency on regex?

It seems to me that the implementation could simply compare the upper and lower-cased versions for equality before warning and it the upper-casing operation certainly could avoid invoking a whole regex machinery.

Contributor

nagisa commented Dec 21, 2017

Is it something that needs a dependency on regex?

It seems to me that the implementation could simply compare the upper and lower-cased versions for equality before warning and it the upper-casing operation certainly could avoid invoking a whole regex machinery.

@varkor

This comment has been minimized.

Show comment
Hide comment
@varkor

varkor Dec 21, 2017

Contributor

The regex dependency could be replaced by explicit loops over pairs of characters in is_camel_case and to_camel_case, though it seemed much less elegant/readable, and less amenable to change if the method ever needed adjustment in the future. I can change it if the extra dependency is not worth it, though.

I'm not quite sure what you mean by comparing the upper and lower cased versions. You mean comparing strings to their to_camel_case results to check is_camel_case? There are some strings that are counted as camel case, but are not fixed points for is_camel_case at the moment: e.g. to_camel_case("FooBar") == "Foobar". This behaviour could also be changed, but would be another change, that's not necessarily desirable.

Contributor

varkor commented Dec 21, 2017

The regex dependency could be replaced by explicit loops over pairs of characters in is_camel_case and to_camel_case, though it seemed much less elegant/readable, and less amenable to change if the method ever needed adjustment in the future. I can change it if the extra dependency is not worth it, though.

I'm not quite sure what you mean by comparing the upper and lower cased versions. You mean comparing strings to their to_camel_case results to check is_camel_case? There are some strings that are counted as camel case, but are not fixed points for is_camel_case at the moment: e.g. to_camel_case("FooBar") == "Foobar". This behaviour could also be changed, but would be another change, that's not necessarily desirable.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 1, 2018

Contributor

☔️ The latest upstream changes (presumably #46278) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Jan 1, 2018

☔️ The latest upstream changes (presumably #46278) made this pull request unmergeable. Please resolve the merge conflicts.

@nagisa

This comment has been minimized.

Show comment
Hide comment
@nagisa

nagisa Jan 1, 2018

Contributor

Sorry for delay. Right, I agree regarding to_camel_case not being a fixed-point.

Regex dependency still bothers me. It is a pretty large dependency to pull into the compiler for such a minor improvement and I feel that we’d be better off investing into a naiver implementation.

Contributor

nagisa commented Jan 1, 2018

Sorry for delay. Right, I agree regarding to_camel_case not being a fixed-point.

Regex dependency still bothers me. It is a pretty large dependency to pull into the compiler for such a minor improvement and I feel that we’d be better off investing into a naiver implementation.

@varkor

This comment has been minimized.

Show comment
Hide comment
@varkor

varkor Jan 2, 2018

Contributor

Okay, I've switched to a more explicit approach.

Contributor

varkor commented Jan 2, 2018

Okay, I've switched to a more explicit approach.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 4, 2018

Member

ping @nagisa, mind taking a look at the updated changes?

Member

alexcrichton commented Jan 4, 2018

ping @nagisa, mind taking a look at the updated changes?

@nagisa

This comment has been minimized.

Show comment
Hide comment
@nagisa

nagisa Jan 4, 2018

Contributor

This seems great to me. Thanks!

@bors r+

Contributor

nagisa commented Jan 4, 2018

This seems great to me. Thanks!

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 4, 2018

Contributor

📌 Commit 3dff918 has been approved by nagisa

Contributor

bors commented Jan 4, 2018

📌 Commit 3dff918 has been approved by nagisa

kennytm added a commit to kennytm/rust that referenced this pull request Jan 4, 2018

Rollup merge of rust-lang#46907 - varkor:contrib-8, r=nagisa
Allow non-alphabetic underscores in camel case

Certain identifiers, such as `X86_64`, cannot currently be unambiguously represented in camel case (`X8664`, `X86_64`, `X8_664`, etc. are all transformed to the same identifier). This change relaxes the rules so that underscores are permitted between two non-alphabetic characters under `#[forbid(non_camel_case_types)]`. Fixes rust-lang#34633 and fixes rust-lang#41621.

bors added a commit that referenced this pull request Jan 5, 2018

Auto merge of #47195 - kennytm:rollup, r=kennytm
Rollup of 10 pull requests

- Successful merges: #46907, #47030, #47033, #47110, #47142, #47149, #47150, #47160, #47173, #47182
- Failed merges:
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 5, 2018

Contributor

⌛️ Testing commit 3dff918 with merge 8a11b8c...

Contributor

bors commented Jan 5, 2018

⌛️ Testing commit 3dff918 with merge 8a11b8c...

bors added a commit that referenced this pull request Jan 5, 2018

Auto merge of #46907 - varkor:contrib-8, r=nagisa
Allow non-alphabetic underscores in camel case

Certain identifiers, such as `X86_64`, cannot currently be unambiguously represented in camel case (`X8664`, `X86_64`, `X8_664`, etc. are all transformed to the same identifier). This change relaxes the rules so that underscores are permitted between two non-alphabetic characters under `#[forbid(non_camel_case_types)]`. Fixes #34633 and fixes #41621.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 5, 2018

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 8a11b8c to master...

Contributor

bors commented Jan 5, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 8a11b8c to master...

@bors bors merged commit 3dff918 into rust-lang:master Jan 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment