Skip to content

Formatting the rust-lang/rust repository #80

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
Centril opened this issue May 5, 2019 · 17 comments
Closed

Formatting the rust-lang/rust repository #80

Centril opened this issue May 5, 2019 · 17 comments
Labels
meeting-proposal T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@Centril
Copy link
Contributor

Centril commented May 5, 2019

Meeting proposal info

About this issue

This issue corresponds to a meeting proposal for the compiler team
steering meeting. It corresponds to a possible topic of
discussion. You can read more about the steering meeting procedure
here
.

Comment policy

These issues are meant to be used as an "announcements channel"
regarding the proposal, and not as a place to discuss the technical
details. Feel free to subscribe to updates. We'll post comments when
reviewing the proposal in meetings or making a scheduling decision.
In the meantime, if you have questions or ideas, ping the proposers
on Zulip (or elsewhere).

@eddyb

This comment has been minimized.

@nikomatsakis nikomatsakis added the T-compiler Add this label so rfcbot knows to poll the compiler team label May 10, 2019
@nikomatsakis
Copy link
Contributor

We discussed this in our planning meeting today and decided that we do not need to have a meeting for this. We believe there is general agreement to the plan. However, @Mark-Simulacrum wanted to make sure we uncovered anybody who might have a strong objection, therefore I'm doing a poll:

@rfcbot poll Do you generally approve of this plan to format the repository?

@rfcbot
Copy link
Collaborator

rfcbot commented May 10, 2019

Team member @nikomatsakis has asked teams: T-compiler, for consensus on:

Do you generally approve of this plan to format the repository?

@nikomatsakis
Copy link
Contributor

I'm going to close this issue -- we didn't feel this needed to be a meeting.

@cramertj
Copy link
Member

I actually had a minor question in the meantime: how should we handle the formatting of tests which are testing new syntax that is being introduced in the change, but not yet supported by rustfmt?

@Centril
Copy link
Contributor Author

Centril commented May 16, 2019

@cramertj We can exclude those directories explicitly in some manner (either by not having .enforcefmt in there or by listing paths explicitly).

@Centril
Copy link
Contributor Author

Centril commented May 16, 2019

That actually might be a problem when we use unstable features in the compiler itself... I hope rustfmt handles those gracefully.

@cramertj
Copy link
Member

It does not-- it currently fails to format anything if your code doesn't parse as a valid rust file.

@Centril
Copy link
Contributor Author

Centril commented May 16, 2019

@cramertj ah; but does rustfmt handle nightly features at least for some selection of features?

@cramertj
Copy link
Member

@Centril it handles some things, but obviously if you've just implemented a feature it wouldn't yet support it.

@Centril
Copy link
Contributor Author

Centril commented May 16, 2019

@cramertj Makes sense; this might be a problem in some instances -- which I guess we can fix with opt-outs in case of emergency -- on the other hand, this might also push us to be better at introducing things to rustfmt quicker, which might be a welcome outcome.

@estebank
Copy link

This feels like it would only be a problem when we start using a feature in the compiler itself, not for the tests, and I would argue that having rustfmt support for features that we feel are useful/safe enough for the compiler should be a priority.

@cramertj
Copy link
Member

Are we not planning to require that tests also be formatted?

@Centril
Copy link
Contributor Author

Centril commented May 17, 2019

@cramertj Sure, but we can do opt-outs for specific folders and files for newly introduced features so as to not let development of new features be hindered by lack of initial rustfmt support. It's sort of a catch-22. Rustfmt can't easily add support without the feature implemented and we couldn't test if we didn't opt out without rustfmt support.

@estebank
Copy link

Please make it a self contained comment annotation to opt out in tests, not a centralized opt out file.

@varkor
Copy link
Member

varkor commented May 18, 2019

I would like to see some discussion about the settings we use for rustfmt in rustc, but I definitely think formatting the repository is a good idea overall.

@Zoxc
Copy link

Zoxc commented May 19, 2019

I would like a tool which can format a branch based on say the last commit before the repo is formatted, injecting a commit which formats the repo. So the procedure for updating a branch would be:

  • rebase to the last commit which is unformatted
  • run the tool
  • rebase to master, but skip the commit formatting the repo

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Dec 17, 2019
…Mark-Simulacrum

Enable incremental rustfmt adoption

Enables an incremental rollout of rustfmt usage within the compiler via a granular ignore configuration and automated enforcement. The decision to format the repository was approved in rust-lang/compiler-team#80 (comment).

This PR includes:

* an `[ignore]` section to `rustfmt.toml` including most of the repository
* `./x.py` downloads rustfmt from a specific nightly (we do not pin to beta or stable as we need unstable features)
* an `./x.py fmt [--check]` command which runs cargo-fmt
* `./x.py fmt --check` runs during the same test step as `src/tools/tidy`, on master, but not on beta or stable as we don't want to depend on nightly rustfmt there.
* a commit to format `src/librustc_fs_util` as an initial target and to ensure enforcement is working from the start
bors added a commit to rust-lang/rust that referenced this issue Dec 22, 2019
…crum

Enable incremental rustfmt adoption

Enables an incremental rollout of rustfmt usage within the compiler via a granular ignore configuration and automated enforcement. The decision to format the repository was approved in rust-lang/compiler-team#80 (comment).

This PR includes:

* an `[ignore]` section to `rustfmt.toml` including most of the repository
* `./x.py` downloads rustfmt from a specific nightly (we do not pin to beta or stable as we need unstable features)
* an `./x.py fmt [--check]` command which runs cargo-fmt
* `./x.py fmt --check` runs during the same test step as `src/tools/tidy`, on master, but not on beta or stable as we don't want to depend on nightly rustfmt there.
* a commit to format `src/librustc_fs_util` as an initial target and to ensure enforcement is working from the start
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
meeting-proposal T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

8 participants