Skip to content

cargo fmt #771

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

Merged
merged 14 commits into from
Jun 13, 2017
Merged

cargo fmt #771

merged 14 commits into from
Jun 13, 2017

Conversation

vignesh-sankaran
Copy link
Contributor

@vignesh-sankaran vignesh-sankaran commented Jun 12, 2017

Fixes #574. Ran rustfmt over the codebase, went smoothly except for this line here (https://github.com/rust-lang/crates.io/blob/master/src/db.rs#L66), but I think that there isn't much we're going to be able to do about that

@elliotekj
Copy link
Contributor

Hey @vignesh-sankaran :)

I have a few suggestions for this:

  1. A rustfmt config file should probably be added to the codebase as part of this PR for consistency going forward. The RFC rustfmt config was suggested by @steveklabnik — was that the one you used?

  2. Rustfmt has a skip attribute which should let us work around the long function name from the OpenSSL crate.

  3. Again for consistency going forward, the Travis config should probably be updated as part of this PR to fail if code isn't formatted correctly: https://github.com/rust-lang-nursery/rustfmt#checking-style-on-a-ci-server

Just my suggestions — best to wait and see what @carols10cents has to say before acting on anything.

Cheers!

@carols10cents
Copy link
Member

Yup, I agree with @elliotekj's suggestions-- do you mind making those changes @vignesh-sankaran? Please let me know if you have any questions!

@vignesh-sankaran
Copy link
Contributor Author

vignesh-sankaran commented Jun 13, 2017

In response @elliotekj, I just used rustfmt with overwrite, I'll redo the rustfmt pass with the config file. Thanks for the tip regarding the skip attribute, I'll add that in too.

No problems @carols10cents, looks like I'll fix up #575 in the process as well.

EDIT: Discovered that #[rustfmt_skip] isn't going to stop rustfmt from checking the line length based off what I've seen here, and it appears to be intended behaviour. As a workaround I've set the max_length to 140 in the .rustfmt.toml, is that ok?

@carols10cents carols10cents merged commit 91898fa into rust-lang:master Jun 13, 2017
@carols10cents
Copy link
Member

carols10cents commented Jun 13, 2017

Thank you so much!! I've merged this in, and I decided to move the documentation to CONTRIBUTING.md and I added some notes about jslint and clippy too.

I also was able to get the max_width down to 118, but then I discovered the error_on_line_overflow setting, which I think does about what we want here, since the line with the super long SSL function can't be fixed or ignored. That also let me change the width to 100, which has rustfmt wrap things more nicely IMO.

I also changed the default write mode to Overwrite, since that's what I usually want when I run locally.

Thank you!! I'm so excited to be able to stop agonizing over the formatting!!!

@vignesh-sankaran
Copy link
Contributor Author

No worries @carols10cents, good to hear you'll be stressing less over code formatting :)

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

Successfully merging this pull request may close these issues.

3 participants