Skip to content

Run rustfmt on the whole codebase and send in the changes #574

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

Closed
carols10cents opened this issue Feb 24, 2017 · 8 comments
Closed

Run rustfmt on the whole codebase and send in the changes #574

carols10cents opened this issue Feb 24, 2017 · 8 comments

Comments

@carols10cents
Copy link
Member

I know I've been adding code to crates.io that isn't necessarily idiomatic rust formatting and I'd like to stop thinking about it and just run rustfmt :) There's probably a lot of code that isn't formatted the way rustfmt would do it though!!

This bug involves:

  • Installing rustfmt on your machine if you haven't yet, using the "Quick start" instructions in their README
  • Run cargo fmt in your crates.io checkout
  • Send in a PR for whatever it recommends changing
  • Fix the tests if any fail (hopefully they shouldn't!)
@ghost
Copy link

ghost commented Feb 24, 2017

1 PR for each change that rustfmt suggests, 1 per file, or a global one ?

@carols10cents
Copy link
Member Author

I think one PR with one commit that makes all the changes that rustfmt suggests in all the files is probably the easiest thing, although I haven't worked with rustfmt much. Is there an easy way to turn off all suggestions, then turn them on one at a time and make one commit per type of suggestion? That might be slightly easier to review, and would make it easier to see what caused any test failures (by using git bisect), should there happen to be any.

Wdyt?

@elliotekj
Copy link
Contributor

elliotekj commented Feb 26, 2017

I happened to be looking at this one at the same time @Insomgla first commented. rustfmt can automatically fix all but 2 mis-formattings in the project. Of those two it can't fix, one involves renaming a function call to get under a 100 character line count (this one if memory serves https://github.com/rust-lang/crates.io/blob/master/src/db.rs#L61) but I'm not sure that can be fixed because the function is from the OpenSSL crate (https://github.com/sfackler/rust-openssl/blob/master/openssl/src/ssl/connector.rs#L112) and the other seemed to be a bug in rustfmt which couldn't correct a trailing whitespace mis-format, yet if it was corrected manually then rustfmt would add the whitespace back in and show an error again. It was late so I didn't look into it too closely and could be wrong though.

All that to say that my suggestion, FWIW, would be 1 PR with 1 commit for the auto-fixes then 1 commit per manual fix as they are not of the same kind.

@ghost
Copy link

ghost commented Feb 26, 2017

@elliotekj I fixed the second misformatting by removing the comment. As for the first one, I agree, we can't do much, except if Rust allowed to split function names :)

@elliotekj
Copy link
Contributor

It seems rustfmt has a skip attribute (https://github.com/rust-lang-nursery/rustfmt/blob/master/tests/target/skip.rs). As the call can't be renamed or reformatted, that looks like the best option to keep travis happy to me. Someone more familiar with the codebase may well have a better suggestion though.

@carols10cents
Copy link
Member Author

It seems rustfmt has a skip attribute (https://github.com/rust-lang-nursery/rustfmt/blob/master/tests/target/skip.rs). As the call can't be renamed or reformatted, that looks like the best option to keep travis happy to me. Someone more familiar with the codebase may well have a better suggestion though.

Yep, sounds like a place to use rustfmt's skip attribute!

@ghost
Copy link

ghost commented Feb 26, 2017

I'm on holidays, so feel free to do it @elliotekj :p

@ghost
Copy link

ghost commented Feb 28, 2017

I created a PR, yet there are still things to discuss. @carols10cents, should we use the default rustfmt formatting (which mostly covers the Rust styles guidelines [but only the ones which are covered by the documentation]) , or do you have anything particular in mind as for the formatting ? Because rustfmt supports configuration files for those "unruled" parts.

At the moment, I sticked to the the default ones, but maybe you have something else in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants