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

Fix camel case type warning for types with trailing underscores #54101

Merged
merged 1 commit into from Sep 19, 2018

Conversation

@osa1
Contributor

osa1 commented Sep 10, 2018

Fixes #54099

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Sep 10, 2018

Collaborator

r? @matthewjasper

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

Collaborator

rust-highfive commented Sep 10, 2018

r? @matthewjasper

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

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Sep 10, 2018

Member

r? @nikomatsakis cc @oli-obk @Manishearth It's not clear whether we should do this.

Member

eddyb commented Sep 10, 2018

r? @nikomatsakis cc @oli-obk @Manishearth It's not clear whether we should do this.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Sep 10, 2018

Member

I see no reason not to, there's no obvious way to "fix" this aside from inventing a new name

Member

Manishearth commented Sep 10, 2018

I see no reason not to, there's no obvious way to "fix" this aside from inventing a new name

@@ -43,8 +43,6 @@ struct foo7 {
bar: isize,
}
type __ = isize; //~ ERROR type `__` should have a camel case name such as `CamelCase`

This comment has been minimized.

@oli-obk

oli-obk Sep 10, 2018

Contributor

Well... The former message was not wrong per se. But pretty much useless

We should probably just add underscore-only identifiers to the https://rust-lang-nursery.github.io/rust-clippy/master/index.html#just_underscores_and_digits lint in clippy

@oli-obk

oli-obk Sep 10, 2018

Contributor

Well... The former message was not wrong per se. But pretty much useless

We should probably just add underscore-only identifiers to the https://rust-lang-nursery.github.io/rust-clippy/master/index.html#just_underscores_and_digits lint in clippy

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 10, 2018

Contributor

@osa1 your PR title says:

trailing underscores

but I don't see any test for that, just leading underscores. Perhaps add some more tests?

Contributor

nikomatsakis commented Sep 10, 2018

@osa1 your PR title says:

trailing underscores

but I don't see any test for that, just leading underscores. Perhaps add some more tests?

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 10, 2018

Contributor

Otherwise r=me, I agree that this makes sense to me.

Contributor

nikomatsakis commented Sep 10, 2018

Otherwise r=me, I agree that this makes sense to me.

@osa1

This comment has been minimized.

Show comment
Hide comment
@osa1

osa1 Sep 12, 2018

Contributor

but I don't see any test for that, just leading underscores. Perhaps add some more tests?

Done!

Contributor

osa1 commented Sep 12, 2018

but I don't see any test for that, just leading underscores. Perhaps add some more tests?

Done!

struct __ {}
struct X_ {}
struct X__ {}
struct X___ {}

This comment has been minimized.

@nikomatsakis

nikomatsakis Sep 12, 2018

Contributor

looking around, I couldn't find many tests for this lint really...

@nikomatsakis

nikomatsakis Sep 12, 2018

Contributor

looking around, I couldn't find many tests for this lint really...

This comment has been minimized.

@nikomatsakis

nikomatsakis Sep 12, 2018

Contributor

e.g., to see whether X_X would fail

@nikomatsakis

nikomatsakis Sep 12, 2018

Contributor

e.g., to see whether X_X would fail

This comment has been minimized.

@osa1

osa1 Sep 12, 2018

Contributor

Hmm I actually don't know the answer to that myself -- that is whether X_X is camel case or not. Btw there are a few more tests about this in lint-non-camel-case-types.rs.

@osa1

osa1 Sep 12, 2018

Contributor

Hmm I actually don't know the answer to that myself -- that is whether X_X is camel case or not. Btw there are a few more tests about this in lint-non-camel-case-types.rs.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 12, 2018

Contributor

Anyway, I'm inclined to r+ this PR -- @oli-obk, @Manishearth, do we think we need an FCP here? I guess we could. Maybe I'll nominate for lang-team to discuss on Thu.

Contributor

nikomatsakis commented Sep 12, 2018

Anyway, I'm inclined to r+ this PR -- @oli-obk, @Manishearth, do we think we need an FCP here? I guess we could. Maybe I'll nominate for lang-team to discuss on Thu.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Sep 12, 2018

Member
Member

Manishearth commented Sep 12, 2018

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 13, 2018

Contributor

We chatted in the lang team mtg and decided "oh what the heck we'll FCP it".

Contributor

nikomatsakis commented Sep 13, 2018

We chatted in the lang team mtg and decided "oh what the heck we'll FCP it".

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 13, 2018

Contributor

I believe @Centril's exact quote was "I have never cared less about anything"

Contributor

nikomatsakis commented Sep 13, 2018

I believe @Centril's exact quote was "I have never cared less about anything"

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 13, 2018

Contributor

@rfcbot fcp merge

I propose we allow any number of trailing or leading underscores because...why not.

Contributor

nikomatsakis commented Sep 13, 2018

@rfcbot fcp merge

I propose we allow any number of trailing or leading underscores because...why not.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 13, 2018

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Sep 13, 2018

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 13, 2018

Member

🤷‍♂️ 👍

Member

joshtriplett commented Sep 13, 2018

🤷‍♂️ 👍

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Sep 13, 2018

Contributor

😪 💤 🛌
👍

Contributor

Centril commented Sep 13, 2018

😪 💤 🛌
👍

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 18, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Sep 18, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Sep 18, 2018

Member

Do we approve now, or wait some more?

Member

eddyb commented Sep 18, 2018

Do we approve now, or wait some more?

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 18, 2018

Member

@eddyb Go ahead.

Member

joshtriplett commented Sep 18, 2018

@eddyb Go ahead.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Sep 18, 2018

Member

@bors r=nikomatsakis

Member

eddyb commented Sep 18, 2018

@bors r=nikomatsakis

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 18, 2018

Contributor

📌 Commit 07646bb has been approved by nikomatsakis

Contributor

bors commented Sep 18, 2018

📌 Commit 07646bb has been approved by nikomatsakis

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 19, 2018

Contributor

⌛️ Testing commit 07646bb with merge 4f3ff5a...

Contributor

bors commented Sep 19, 2018

⌛️ Testing commit 07646bb with merge 4f3ff5a...

bors added a commit that referenced this pull request Sep 19, 2018

Auto merge of #54101 - osa1:issue_54099, r=nikomatsakis
Fix camel case type warning for types with trailing underscores

Fixes #54099
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 19, 2018

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 4f3ff5a to master...

Contributor

bors commented Sep 19, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 4f3ff5a to master...

@bors bors merged commit 07646bb into rust-lang:master Sep 19, 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