Disallow stylistic lint

[TisButMe]

See #11432

[brson]

Adding more of these would be a good newbie project #3070. Just coming up with new ideas for them and putting them in the issue would be good.

[huonw]

Does this bootstrap without some library changes? (i.e. for now, just adding #[allow(...)] to the crates that complain, we can make the adjustments progressively.)

[TisButMe]

The next round of fixes to do occurs in macros expansions, such as:

static $name: ::std::local_data::Key<$ty> = &::std::local_data::Key;

which is not recognized as uppercase. I'm not sure what the smart way to fix these is, however.

EDIT: I could disable checking if the ident starts with $ in lint.rs. Starting a variable name with $ seems to be illegal, so I can't think of a bad side-effect.

EDIT2: The lint check seems to occur after macro expansion, which means the args of the macro have to be uppercase. This means changing the macro:

declare_special_idents_and_keywords!

Not sure I want to meddle with this...

[brson]

This looks pretty sweet, and I like that it already caught a lot of stylistic violations. I don't quite understand what's blocking. Is one of our macros generating incorrect identifiers?

[TisButMe]

I'm not very familiar with macros expansion in rust, so I can only guess.

Some macros seem to define static variables with things like $name, where $name is provided by the macro use. In some macro, the identifier provided is not UPPER_CASE, so after macro expansion it fails.

The macro that stops make check now is one that (seems to) define the language keywords. I have not put enough time in it to really realize if it's safe to change its (only, as far as I and grep can tell) use.

[huonw]

It's probably ok to just put #[allow(non_uppercase_statics)]; on the token.rs file so that this can land, and others can go and fix up the little bits and pieces later.

(Also, this needs a rebase.)

[TisButMe]

So, I put the #[allow(non_uppercase_statics)] on top of the complaining files, but somehow it still doesn't pass a make check ?

I'm unsure where to go from there

[lucy]

I think the allow attributes have to either be added before every item that violates the lint or have a leading semicolon. E.g.

#[allow(non_camel_case_types)];

type a_not_camel_case = bool;
type b_not_camel_case = bool;

works, while

#[allow(non_camel_case_types)]

type a_not_camel_case = bool;
type b_not_camel_case = bool;

only allows non-camel-case-types for the a_not_camel_case type.

[TisButMe]

Thanks, it now passes that stage, opening up another lot of fixes to be made.

[alexcrichton]

Can you add an exception for this? It should really be fd_t

[alexcrichton]

Nice work, and thanks!

Could you also rebase the commits into two stages? The first is enabling the lints and the second would just be dealing with the fallout.

[TisButMe]

Just to be sure, (not so good at git yet), you want me to move HEAD to the commit enabling the lints, do a rebase there, and then move back to the current stage and rebase too ?

[alexcrichton]

I normally rebase by using git rebase -i <some commit before your first commit> and then you can instruct git how to squash things (and even re-order)

[TisButMe]

Is this what you wanted ?

[alexcrichton]

I think this ended up picking up a lot of extra commits from all over the place. If you take a look at github's commits tab you can tell if it's gotten to the right point.

[adrientetar]

Looks like it worked but the branch history is bloated. I would start from a clean branch and cherry-pick the two squashed commits, d910867 and b64697d.

[TisButMe]

Agreed. Is there any better way to do that than closing this PR, creating a new branch, rebase -i on this one removing every commit except these 2, and the PR-ing again ?

[adrientetar]

Here is what I would do:
checkout to master: git checkout master
create a new working branch: git checkout -b new
cherry-pick commits: git cherry-pick d910867 && git cherry-pick b64697d
swap branches: git branch -m disallow_stylistic_lint old && git branch -m new disallow_stylistic_lint
Then a push and it should be good to go.

[adrientetar]

Note that you can probably achieve that with by resetting the branch but starting from a clean one is always guaranteed to succeed.

(The other way around would be git reset <LAST_GOOD_COMMIT> && git checkout . && git clean -fd then cherry-picking.)

[TisButMe]

The branch should now be clean. After swapping, the remote branch was ahead from the (swapped and cleaned) local one, and pulling would have included all the removed useless commits (right ?) so I deleted the remote, and pushed again. Are there better ways ?

[adrientetar]

@TisButMe Sorry I forgot to precise, you have to use --force to overwrite remote when it is ahead i.e. git push --force origin <branch>.

[TisButMe]

@adridu59 Thanks ! I'll do it the right way next time then :)

[huonw]

There are still 7 "junk" commits, from what I can see.

[TisButMe]

There we go.

[adrientetar]

@TisButMe I think you have to rebase against master since #11001 renamed iterators, hence the build errors.

[huonw]

This needs a rebase.

[alexcrichton]

Closing due to inactivity, but feel free to open with a rebase!

[brson]

Hope this gets picked up again!

[TisButMe]

Sorry for the lack of activity, I am a bit busy at the moment. I'll try to
come back to it when I have a little more free time :)

2014-02-06 Brian Anderson notifications@github.com:

Hope this gets picked up again!

Reply to this email directly or view it on GitHubhttps://github.com//pull/11457#issuecomment-34287818
.

[mrshu]

I tried to go for this in the meantime and by just setting NonCamelCaseTypes to warn make check ended up with 330 errors.

What do you think the correct approach to this should be? I'd like to continue working on this. Should I send a work in progress pull request and temporarily put #[allow(non_camel_case_types)] everywhere?

[brson]

@mrshu Were they legitimate violations or are there 330 places where we really need to violate the style guide?

[mrshu]

@brson There are things like X86_64_Win64 or BasicBlock_opaque where I see the logic behind the naming. Also, I noticed a few _indenter and things like general_const and const_val which probably make sense too.

So to answer your question, I do not really know.

[brson]

I guess then I'd say turn it on and put #[allow(non_camel_case_types)] at the smallest scope possible to contain the problem. rustc and syntax are an exception because they are ancient and follow a variety of coding styles, so you might as well just allow the bogosity for the entire crates until somebody cleans them up.

[mrshu]

@brson thanks for clarification.

Here is what I got btw: http://paste.pm/e1a.js

[TisButMe]

To anyone wanting to restart work on this: there are already files that
have been updated to fit the "correct" style in this PR-related files, so
before putting #[allow()] everywhere maybe check if I didn't fix that
particular occurrence already.

2014-02-14 0:13 GMT+01:00 Marek Šuppa notifications@github.com:

@brson https://github.com/brson thanks for clarification.

Here is what I got btw: http://paste.pm/e1a.js

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/11457#issuecomment-35038123
.

[mrshu]

Thanks @TisButMe. I am right now just following @brson's comment and trying to clean up rustc and syntax. I'll merge your work when I get there.