Skip to content
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

Format our code with rustfmt #8553

Closed
tetsuharuohzeki opened this issue Nov 16, 2015 · 12 comments
Closed

Format our code with rustfmt #8553

tetsuharuohzeki opened this issue Nov 16, 2015 · 12 comments
Labels

Comments

@tetsuharuohzeki
Copy link
Member

@tetsuharuohzeki tetsuharuohzeki commented Nov 16, 2015

I think we should do this.

@tetsuharuohzeki
Copy link
Member Author

@tetsuharuohzeki tetsuharuohzeki commented Nov 16, 2015

cc: @Manishearth ?

@jdm
Copy link
Member

@jdm jdm commented Nov 16, 2015

Ms2ger and I have filed a lot of issues each time we've tried rustfmt on Servo. We could certainly start running it on particular files that don't end up with terrible output.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Nov 16, 2015

It's not ready yet. I periodically try running it on Servo.

@frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Nov 18, 2015

Anyone know how Go projects incorporate gofmt (or Rust w/ rustfmt) into project workflows? Require that it get used pre-commit and checked via CI? Have the build system automatically format upon check-in (which sounds scary)? Something else?

@tetsuharuohzeki
Copy link
Member Author

@tetsuharuohzeki tetsuharuohzeki commented Nov 18, 2015

I think it would be better to check in CI the commited files as tidy which we're running. It's not enough?

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Nov 18, 2015

Checked via CI I think. Similar to how clippy checks update_lints or how servo checks lockfile_changed.

I'd rather not gate CI on rustfmt though. I'm slightly more okay with it if we use circleci or something for tidy+fmt to get separate and immediate status icons from the long build.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Jan 2, 2017

So I've been poking around, and rustfmt seems to be much more stable now.

The following toml config is pretty nice for servo

reorder_imports = true
reorder_imported_names = true
max_width = 120
ideal_width = 80
normalize_comments = false

normalize_comments prevents the license comment from being reformatted to use line comments instead of block comments. We might want to do that anyway, but it's nice to reduce the impact of the first run.

rustfmt currently doesn't reorder extern crates. Furthermore, it reformats #[attr] extern crate foo; into two lines, which breaks tidy (tidy only asks for the sorting of extern crate lines which do not have attributes, since the ones with attributes usually have special requirements for ordering). We'll need to change this a bit and/or add extern crate sorting to rustfmt.

I would like to reformat everything and slowly make rustfmt part of CI checks. Perhaps removing the corresponding tidy checks.

Thoughts? @jdm @Ms2ger @wafflespeanut

@jdm
Copy link
Member

@jdm jdm commented Jan 2, 2017

I'm open to that plan. I think adding extern crate reordering to rustfmt makes the most sense.

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Jan 3, 2017

It'd be wonderful to have rustfmt in tree. Also, tidy's sorting isn't that great right now (#9259). We can improve it in tidy, but rustfmt should do this better. I never imagined that rustfmt could be integrated so easily 😄

bors-servo added a commit that referenced this issue Jan 3, 2017
Rustfmt script_traits and net_traits.

CC #8553.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14831)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jan 4, 2018
Format parts of layout

Formats the following files:
* components/layout/display_list_builder.rs
* components/layout/webrender_helpers.rs

Remove outdated options from rustfmt.toml.
Configure rustfmt to place binary operators
at the end of line (to match ./mach test-tidy).

Rationale: I am tired of indenting my patches by hand trying my best to do it correctly and match the surrounding code. Just to be told that either my indentation is wrong or that I should switch to block indentation for this function because it looks better.

The new formatting passes `./mach test-tidy`. Compared to the old formatting it is a lot more consistent but also tends to spread the code across more lines. The diff is this big because a lot of code used visual indentation.

See also #8553

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19681)
<!-- Reviewable:end -->
@pyfisch
Copy link
Contributor

@pyfisch pyfisch commented Jan 20, 2018

🎉 rustfmt now works well on servo with this config:

match_block_trailing_comma = true
binop_separator = "Back"
error_on_line_overflow = false

It can be installed with rustup install rustfmt-preview for servo and run with cargo fmt --package layout for individual packages like layout.

The formatting works with ./mach test-tidy but the long (>100 character) imports need to be formatted by hand.

@jdm jdm mentioned this issue Aug 9, 2018
53 of 53 tasks complete
@atouchet
Copy link
Contributor

@atouchet atouchet commented Nov 7, 2018

Is this done now?

@CYBAI
Copy link
Collaborator

@CYBAI CYBAI commented Nov 7, 2018

Done by #22126 !

@CYBAI CYBAI closed this Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.