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 all code with rustfmt #21373

Closed
jdm opened this Issue Aug 9, 2018 · 17 comments

Comments

Projects
None yet
9 participants
@jdm
Member

jdm commented Aug 9, 2018

#8553 (comment) describes a configuration that respects some style settings that we care about. I think it's worth systematically formatting Servo's code crate by crate (or module by module in the case of particularly large crates) and then integrating the changes in #20617 to enforce proper style in our CI.

Components:

ports:

@highfive

This comment has been minimized.

highfive commented Aug 10, 2018

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@jdm

This comment has been minimized.

Member

jdm commented Aug 10, 2018

We now have a ./mach rustfmt [path] command that can be used, and you'll want to run rustup component add rustfmt-preview to install rustfmt if you don't already have it.

@kingdido999

This comment has been minimized.

Contributor

kingdido999 commented Aug 11, 2018

Hi, I would like to work on this and will probably start from formatting the first component on the top.

@highfive: assign me

@highfive

This comment has been minimized.

highfive commented Aug 11, 2018

Hey @kingdido999! Thanks for your interest in working on this issue. It's now assigned to you!

@highfive highfive added the C-assigned label Aug 11, 2018

@kingdido999

This comment has been minimized.

Contributor

kingdido999 commented Aug 11, 2018

So I've successfully built the project and ran rustup component add rustfmt-preview successfully. But when I do ./mach rustfmt components/allocator/lib.rs, it says:

It looks like you passed an unrecognized argument into mach.

The rustfmt command does not accept the arguments: components/allocator/lib.rs

So I look into the rustfmt command defined in python/servo/devenv_commands.py #21374 , seems we have to pass in a --directory argument. Then I do ./mach rustfmt -d components/allocator, it returns:

Error: `/Users/Desmond/repo/servo/components/allocator` is a directory

Despite the error, it does run the formatting but for all files in the project, which is not what we want. If I provide a file path instead of a directory path, no error is shown but it formats the entire project again. My best guess is that --directory is not a valid command for rustfmt, we could produce the same error if we run rustfmt components/allocator.

Directly run rustfmt against a file works properly:

rustfmt components/allocator/lib.rs

Here are the versions of rustc and rustfmt on my system:

$ rustc --version
rustc 1.29.0-nightly (1ecf6929d 2018-07-16)

$ rustfmt --version
rustfmt 0.8.2-nightly (5e599251 2018-07-02)

@jdm @JoshBrudnak Please let me know if I'm missing something when I do ./mach rustfmt [path], thanks!

@jdm

This comment has been minimized.

Member

jdm commented Aug 14, 2018

I've verified that you're not missing anything. cargo fmt does not appear to do what we want; it looks like we'll need to modify the ./mach rustfmt command to recursively find all of the rust source files under the given directory and pass those as arguments to rustfmt directly. Would you like to make that change?

@kingdido999

This comment has been minimized.

Contributor

kingdido999 commented Aug 15, 2018

Sure I will work on it.

@kingdido999

This comment has been minimized.

Contributor

kingdido999 commented Aug 15, 2018

I'm trying to get a rough idea on the use cases of /mach rustfmt [directory] other than resolving this particular issue (format crate one at a time). It seems to me that we can simply get away with rustfmt as a part of future CI build after all crates have been formatted. So if that is true, we don't necessarily need to invent an extra command to format a directory, we could do something like this instead:

rustfmt ports/servo/**/*.rs
@jdm

This comment has been minimized.

Member

jdm commented Aug 15, 2018

Sure, that would work. I meant to make this issue possible for multiple people to work on in parallel since there are so many components, so I figured a mach command would be the easiest way to support that.

kingdido999 added a commit to kingdido999/servo that referenced this issue Aug 16, 2018

@kingdido999 kingdido999 referenced this issue Aug 16, 2018

Merged

Format component allocator #21428

2 of 4 tasks complete

bors-servo added a commit that referenced this issue Aug 16, 2018

Auto merge of #21428 - kingdido999:rustfmt/allocator, r=jdm
Format component allocator

<!-- Please describe your changes on the following line: -->
Format `components/allocator` with:

```bash
rustfmt components/allocator/*.rs
```

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors. Waiting for #21423 to be resolved.
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix a part of #21373.
- [x] These changes do not require tests because they format the code only.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21428)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this issue Aug 16, 2018

Auto merge of #21428 - kingdido999:rustfmt/allocator, r=jdm
Format component allocator

<!-- Please describe your changes on the following line: -->
Format `components/allocator` with:

```bash
rustfmt components/allocator/*.rs
```

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors. Waiting for #21423 to be resolved.
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix a part of #21373.
- [x] These changes do not require tests because they format the code only.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21428)
<!-- Reviewable:end -->

kingdido999 added a commit to kingdido999/servo that referenced this issue Aug 17, 2018

bors-servo added a commit that referenced this issue Aug 17, 2018

Auto merge of #21440 - kingdido999:rustfmt/atoms, r=jdm
Format components atoms

<!-- Please describe your changes on the following line: -->

```bash
rustfmt components/atoms/*.rs
```

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix a part of #21373 .
- [x] These changes do not require tests because it's code format change.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21440)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this issue Aug 17, 2018

Auto merge of #21442 - kingdido999:rustfmt/bluetooth, r=jdm
Format components bluetooth and bluetooth_traits

<!-- Please describe your changes on the following line: -->

```bash
rustfmt components/bluetooth/*.rs
rustfmt components/bluetooth_traits/*.rs
```
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix a part of #21373 .
- [x] These changes do not require tests because it's code format change.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21442)
<!-- Reviewable:end -->
@chansuke

This comment has been minimized.

Contributor

chansuke commented Aug 19, 2018

Hi.I'd like to work on this too.I will start from the component on the bottom:)

kingdido999 added a commit to kingdido999/servo that referenced this issue Aug 30, 2018

bors-servo added a commit that referenced this issue Aug 30, 2018

Auto merge of #21538 - kingdido999:master, r=jdm
Format components canvas and canvas_traits #21373

<!-- Please describe your changes on the following line: -->

```bash
rustfmt components/canvas/**/*.rs
rustfmt components/canvas_traits/**/*.rs
```

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21538)
<!-- Reviewable:end -->

kingdido999 added a commit to kingdido999/servo that referenced this issue Aug 31, 2018

bors-servo added a commit that referenced this issue Aug 31, 2018

Auto merge of #21551 - kingdido999:master, r=jdm
Format component compositing #21373

<!-- Please describe your changes on the following line: -->

```bash
rustfmt components/compositing/*.rs
```

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21551)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this issue Aug 31, 2018

Auto merge of #21551 - kingdido999:master, r=jdm
Format component compositing #21373

<!-- Please describe your changes on the following line: -->

```bash
rustfmt components/compositing/*.rs
```

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21551)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this issue Sep 10, 2018

Auto merge of #21651 - AnshulMalik:format-profile, r=jdm
format components/profile

Issue #21373

<!-- 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/21651)
<!-- Reviewable:end -->

@chansuke chansuke referenced this issue Sep 10, 2018

Merged

Format component servo #21664

3 of 4 tasks complete

bors-servo added a commit that referenced this issue Sep 10, 2018

Auto merge of #21664 - chansuke:format_servo, r=jdm
Format component servo

<!-- Please describe your changes on the following line: -->
Format components with:

`rustfmt components/servo/*.rs`

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix part of #21373.
- [x] These changes do not require tests because

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21664)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this issue Sep 10, 2018

Auto merge of #21655 - kingdido999:master, r=jdm
Format jstraceable_derive #21373

```bash
rustfmt components/jstraceable_derive/*.rs
```

<!-- 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/21655)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this issue Sep 10, 2018

Auto merge of #21657 - AnshulMalik:format-profile_traits, r=jdm
format components/profile_traits

Issue: #21373

<!-- 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/21657)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this issue Sep 10, 2018

Auto merge of #21661 - chansuke:format_size_of_test, r=jdm
Format component size_of_test

<!-- Please describe your changes on the following line: -->
Format `components/size_of_test` by:
```
rustfmt components/size_of_test/*.rs
```
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix part of #21373.
- [x] These changes do not require tests because they format code only.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21661)
<!-- Reviewable:end -->

kingdido999 added a commit to kingdido999/servo that referenced this issue Sep 11, 2018

kingdido999 added a commit to kingdido999/servo that referenced this issue Sep 11, 2018

bors-servo added a commit that referenced this issue Sep 11, 2018

Auto merge of #21672 - kingdido999:master, r=jdm
Format components metrics and msg #21373

```bash
rustfmt components/metrics/lib.rs
rustfmt components/msg/*.rs
rustfmt components/msg/tests/*.rs
```

<!-- 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/21672)
<!-- Reviewable:end -->

@chansuke chansuke referenced this issue Sep 11, 2018

Merged

Format ports/libsimpleservo #21675

3 of 4 tasks complete

bors-servo added a commit that referenced this issue Sep 11, 2018

Auto merge of #21675 - chansuke:format_libsimpleservo, r=jdm
Format ports/libsimpleservo

<!-- Please describe your changes on the following line: -->
Format `ports/libsimpleservo` with:

`rustfmt ports/libsimpleservo/*.rs`

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix part of #21373.
- [x] These changes do not require tests because they format code only.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21675)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this issue Sep 11, 2018

Auto merge of #21672 - kingdido999:master, r=jdm
Format components metrics and msg #21373

```bash
rustfmt components/metrics/lib.rs
rustfmt components/msg/*.rs
rustfmt components/msg/tests/*.rs
```

<!-- 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/21672)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this issue Sep 12, 2018

Auto merge of #21672 - kingdido999:master, r=jdm
Format components metrics and msg #21373

```bash
rustfmt components/metrics/lib.rs
rustfmt components/msg/*.rs
rustfmt components/msg/tests/*.rs
```

<!-- 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/21672)
<!-- Reviewable:end -->
@AnshulMalik

This comment has been minimized.

Contributor

AnshulMalik commented Sep 12, 2018

rand is already formatted.

bors-servo added a commit that referenced this issue Sep 13, 2018

Auto merge of #21693 - chansuke:format_script_traits, r=jdm
Format components/script_traits

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix part of #21373.
- [x] These changes do not require tests because format the code only.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21693)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this issue Sep 13, 2018

Auto merge of #21694 - chansuke:format_script_plugins, r=jdm
Format components/script_plugins

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix part of #21373.
- [x] These changes do not require tests because they formatting code only.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21694)
<!-- Reviewable:end -->

@chansuke chansuke referenced this issue Sep 18, 2018

Merged

Format script component #21737

3 of 4 tasks complete

bors-servo added a commit that referenced this issue Sep 19, 2018

Auto merge of #21737 - chansuke:format_script, r=jdm
Format script component

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix part of #21373.
- [x] These changes do not require tests because they format code only.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21737)
<!-- Reviewable:end -->

@atouchet atouchet removed the C-assigned label Oct 6, 2018

@pyfisch

This comment has been minimized.

Contributor

pyfisch commented Oct 15, 2018

Hi, formatting is almost complete. I'd like to suggest that servo adopts a new formatting for use statements. Currently use statements are ordered alphabetically and split into multiple lines if they are longer than 100 characters.

I would want to split the imports into 4 groups (std, third-party imports, workspace imports, imports from the same crate) and sort those groups individually. (See rust-lang-nursery/rustfmt#2979)

Additionally I prefer to use merge_imports formatting. I can see at a glance which items are imported from a module and how the module hierarchy is structured. (example file)

The grouping can be done with a script. This could either be added to cargo fmt or the lines are grouped by a script once.

bors-servo added a commit that referenced this issue Nov 5, 2018

Auto merge of #22099 - pyfisch:net-fmt, r=jdm
Format net and net_traits crates

Manually formatted the fetch_scheme file portion to limit right drift.

Part of #21373

<!-- 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/22099)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this issue Nov 5, 2018

Auto merge of #22099 - pyfisch:net-fmt, r=jdm
Format net and net_traits crates

Manually formatted the fetch_scheme file portion to limit right drift.

Part of #21373

<!-- 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/22099)
<!-- Reviewable:end -->
@Dmitry-Borodin

This comment has been minimized.

Dmitry-Borodin commented Nov 5, 2018

@jdm I see the last components from the list net and net_traits was formatted in #22099.
Should this issue now be closed?
It's among the first options in https://starters.servo.org/ -)

@atouchet

This comment has been minimized.

Contributor

atouchet commented Nov 5, 2018

#20617 still needs to be finished.

@CYBAI

This comment has been minimized.

Collaborator

CYBAI commented Nov 7, 2018

Fixed in #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