@nikomatsakis I noted before that I'd like to get working on a T-Infra+ T-Compiler proposal for our policy re. formatting in the repo; My current idea is something like this:
1. Our goal is:
./x.py fmt will format everything in the repo according to our wants.
./x.py test src/tools/tidy + CI will enforce formatting.
@bors fmt will add a commit to the PR where
./x.py fmt has been executed.
2. Rollout plan:
a. We will start by encoding the formatting rules into
tidy will only require formatting inside directories with a
c. We will incrementally start adding
.enforcefmt files to higher and higher level folders until there's just one top-level
d. We remove the
.enforcefmt file and
tidy now enforces it globally everywhere (possible caveat around submodules).
What do you think? Also, what is the best avenue for discuss the draft before publishing the RFC to rust-lang/rfcs?
cc @Pietro Albini --^ also, can we technically add commits to a PR with
I'd prefer for the automatic command not to be in bors
@Pietro Albini any specific reason (so I can include it in the RFC)?
bors is already pretty fragile and I'd prefer for it to be free of repo-specific commands
I'm not opposed at all to an automated command though, but maybe triagebot would be better
@Pietro Albini yeah; good rationale :slight_smile:
triagebot sounds good since it is centralized as opposed to having too many bots.
(also almost every time we redeploy bors the queue messes up, so it's better to avoid as much changes as possible)
I'd also add
./x.py install-format-hook, which installs pre-commit git-hook for formatting.
btw, I don't think t-infra should be involved in this as a whole
@matklad yeah good idea; we should probably have a script for the rebase fallout
@Pietro Albini oh? curious... I'd expect y'all to be involved in vetting the implementation?
we want infra not to be the team developing bots (for example triagebot is technically under t-release), and the rustbuild stuff is mostly mark and alex
@Pietro Albini ah; good point -- rustbuild is also kennytm no?
@Pietro Albini So just T-compiler then?
maybe? not sure how much kenny works on rustbuild
Also, do we already have
rustfmt.toml we want to use for the repo? I am writing new code for the lexer, and, if I could, I'd like to format it in a good way from the start
maybe chime in the next infra meeting and announce y'all are working on this so the interested members can get into the loop
@matklad doesn't seem so; I would just start with the standard format and tweak as needed
There was a pretty strong desire to use at least
use_small_heuristics = "Max"
Ideally, rust-lang/rust shouldn't have too divergent of a formatting from the standard
@matklad sure, that seems good
So, as a first step, let's maybe commit just
rustfmt.toml, without any CI enforcement? That'll help with writing new code in a good style immediately
Enforcement is 100% required, but it'll be a long time before we will set up all the bots..
Otoh, reformatting once is not a big deal, so it's a small win anyway. Otth, starting without enforcement should help us to iterate and agree on the style more flexibly.
@matklad hopefully it shouldn't take too much time to set this up; just a few E-easy issues :D
also, cc @Vadim Petrochenkov , who is not quite satisfied with default rustfmt style as well
@matklad I believe @Vadim Petrochenkov's primary dissatisfaction was due to
use_small_heuristics; setting it to
Max should be good?
I don't remmeber, better to ask or to dig that internals thread
use_small_heuristics = "Max" and using "rustfmt 2.0 mode" (enables breaking bugfixes) resolved all my concerns.
Also, do we already have
rustfmt.tomlwe want to use for the repo? I am writing new code for the lexer, and, if I could, I'd like to format it in a good way from the start
Hmm. I feel somewhat strongly we should use the defaults. But I guess I would accept
use_small_heuristics = Max, particularly if it means we get rustfmt :)
I'd be opposed to a growing set of tweaks though.
Anyway, all of this sounds very good to me. =)
Hmm. I feel somewhat strongly we should use the defaults.
( To be clear, this has nothing to do with the formatting itself, and everything to do with being simple and uniform. Anyway, I'm good with the compromise. =) )
@nikomatsakis OK; I'll work up a draft then and make a PR against the master of my fork for y'all to review
@centril sounds good -- note that we wound up with a process that uses issues to track meeting proposals; though I think it's fine to prep in a fork and file an issue with a link over there
I guess I didn't think it all through, in that there is a template... well... whatever
Filed https://github.com/rust-lang/rust/pull/60520 just now :D
do whatever :)
@nikomatsakis aight; cool
@nikomatsakis here you go: https://github.com/rust-lang/compiler-team/issues/80 (draft to RFC included there)
@centril what's the current status here? Am I correct that the plan is agreed upon, but
./x.py fmt and
.enforcefmt are not implemented yet? Is there a tracking issue?
@matklad I need to finish up my RFC and publish it
Hm, I thought that the agreement has been already reached? https://github.com/rust-lang/compiler-team/issues/80#issuecomment-491324076
@matklad I believe the specifics still warrants the RFC
Ha, I was just going to ask the exact same question
("what is status")
@nikomatsakis do you think I should file the RFC or perhaps just a tracking issue is sufficient?
I feel like this probably doesn't require an RFC
It seems like we had reached consensus on compiler team
I dont' think an RFC hurts per se, but it's more overhead for sure
Alright; I'll think about what requires least effort on my part then / what I can do more quickly
I personally think the lion share's of the work is figuring out a way to land this smoothly in-tree, but I think mostly x.py fmt is not _too_ hard a change to make (modulo rebasing everyone's PR :)
It's mostly a one-time cost
and I believe circulation in rust-lang/rust is fast enough that rebasing can largely be left up to folks to deal with without a script
@simulacrum should we just go ahead and format the whole repository for now without enforcement?
queue and total # prs is low
I personally think that would be helpful but I'd probably move more gradually, e.g. first land a PR that makes rustfmt be part of the downloaded stage0 bundle, and invocable via x.py fmt on any subtree
and then as a followup use that infrastructure for formatting and enforcement (in that order, I don't know that it matters if it happens simultaneously. Could be good to split up or combine, I could see both options having their own benefits).
@simulacrum would you like to file a tracking issue and such? you seem to have a better idea of the implementation
ah... I'd rather not, no. I probably don't have the will/energy to drive this through myself though I can help review the bootstrap changes
ok, fair; I'll see when I have time to do so although @matklad might be interested to spearhead this?
Can we enforce it on all new PRs? i.e. only for files touched?
Then we can incrementally update the rest
how do we do that technically?
We can get the list of changed files from git
I am fairly strongly against any sort of incremental thing personally since I wouldn't want to have to deal with reverting changes made on save if I enable rustfmt-on-save locally to unrelated PRs
er, unrelated files
yea; and it seems like it will mess with history more
I fairly often save on "accident" so to speak with just muscle memory
Why would you need to revert them?
doing it once seems better
I'd prefer that formatting commits are only formatting commits -- I think we can afford to do it either by-crate or even just a global single PR, authored and landed by someone we trust
(Since reviewing it is impossible)
@simulacrum we could split the formatting of the code folders and the test folders
Sure. I don't care much, just separate PRs, not rolled into normal ones.
but ultimately trust is necessary
Was the issue of rustfmt breaking 100-character limit and other tidy checks resolved, btw?
Not sure; we'll find out quickly tho if tidy complains :slight_smile: