rustfmt #643
rustfmt #643
Conversation
|
Hi, thanks for this PR. It's easy and healthy to run rustfmt every now and then but I don't want to have it block CI in lyon. I appreciate how satisfying it feels to enforce consistent formatting in automation, but forcing everyone to use the tool adds friction and raises the bar for contribution rather than making the project more approachable. We already get most of the benefits of familiar code formatting from running rustfmt from time to time. In a similar line of reasoning (the virtues of familiar formatting), please use the default configuration when running the tool. I don't mean to criticize rustfmt blocking CI in general. It has tradeoffs and its value depends on the size of the project, amount of contributors, frequency of contributions, whether or not there is recurrent conflicts that need diffusing, etc. It makes sense for a lot of projects but isn't worth the extra friction in lyon. |
|
Hmm, I don't really agree. It's an offical tool, so friction is minimal. I assume most people who know rust well enough to contribute to a project like this, already have rustfmt installed and otherwise it's just a small I'm totally fine with using the standard configuration, as this is a matter of personal taste. Nevertheless the default should maybe be make explicit with a Also I think the problem with formatting the whole codebase only now and then after a rather big period of not doing so, just makes the git history very noise e.g. git blame shows contributors which didn't even change the code but only reformatted it. Maybe I can still persuade you... :) If not, then that's also fine. |
|
Maybe another option would also be to just automatically run rustfmt when a PR is being merged, instead of checking if the code was already formatted. Then the contributor doesn't have to worry about it. Zero friction. Although I am not sure, if I like the idea of automatic code alteration by CI, which was not approved by a human. |
It's an official tool, so installing it is easy. Having it as a mandatory part of the process adds friction and it has nothing to do with how official it is.
You'd be surprised. It's not a question of having the tool installed. It's also going through the all of the contributions steps twice (after having received a CI error halfway because renaming a function parameter hit the threshold of one of the heuristics that decide when to break lines). It's double the amount of effort for small/simple pull requests which are the majority of lyon's external contributions. Updates to the rust toolchain often introduce differences in how rustfmt changes your code. That adds another round of figuring out what's going on, updating, fixing, and forces people to stay in sync with the CI's compiler version.
It isn't (not on linux with a fresh install from rustup as far as I can tell).
Again, you'd be surprised. I've had a few contributions from people just learning Rust. Small things of course, but it was valuable for them to have a very simple process. Strict formatting adds extra things to think about. Should I run the tool for every commit? At the end before submitting? Ah darnit, I forgot to do it, now I have to submit another PR and wait another day for nical to wake up and merge it. Actually I was using the wrong toolchain version and so I have to update and do it again. These aren't issues for you because you have integrated this into your workflow and habbits. I don't want to impose this for everyone else. Lyon isn't a 20 years old code base with hundreds of opinionated developers. It's style isn't all over the place and it looks very close to standard rust code. You'll see when running the tool that the main differences are in some heuristics of where to break lines for this or that type of item which are hard for a human to remember and do consistently but have small impact on readability. There are other projects (typically what I do at work), where I totally embrace that the code is automatically formatted. Different project sizes and configurations have different needs.
Yes, this makes sense.
It's okay. I'll just ask that:
If you would like to reformat the code base now so that your future contributions are on top of a freshly formatted repo, go for it. |
|
Okay, you've convinced me somewhat.
Can you also state your opinion on this? I'm going to go forward with just running rustfmt right now, while keeping your requests in mind, and then formatting the code I added myself in my own contributions |
Like you I'm a bit wary of having something change the code transparently without any supervision. Being able to hold off re-formatting avoids annoyance when you have a series of changes that you want to land in separate PRs (inserting code formatting in between is likely to cause rebase conflicts). In my opinion, with lyon's current development the pace, there is little value in having every single commit formatted and it is fine to format the code every now and then manually (it doesn't have to be often). A bot could submit the PRs but that would be more to satisfy a programmer's desire for automation than for real productivity's sake. I am not sure whether the commit hashes are preserved when landing with "Rebase and merge". If so, a simple shell script in the repo could format the code, create the commit, and append the formatting commit hash to a |
|
It is not possible to have |
|
Sorry, looks like rustc nightly doesn't like my suggestion to add It won't work with rustfmt 2.0 but by then I suspect rustfmt will have changed enough that we'll have to figure out how to transition to the new tool. (Edit: or you could #[rustfmt::skip] every test in earcut_test if you prefer) |
|
The file that matters is really earcut_tests because it has some pretty enormous shapes that span over thousands of lines if reformatted. It's fine fine if you reformat fill_tests. |
Stop rustfmt from automatically formatting `tessellation/src/earcut_tests.rs`. This is done by putting `#[rustfmt::skip]` on the module declaration: `#[rustfmt::skip] mod earcut_tests;` This will not work with rustfmt 2.0 anymore.
|
I'm now just skipping the |
|
Thanks for your patience. |
but this disallow contributor that often use rustfmt instead of use their time to add space themself. There is no silver bullet, since you choice this, you choice to reduce potential contribution of people who use rustfmt on daily basic and that a LOT of people. And you will need to accept PR that sometime run rustfmt on whole file or project, this will probably be refused cause you will not want to review this kind of commit and the contributor will not want to make effort to do what a tool already do. Rust fmt is well documented and simple to use, there is no reason to not trust people ability to learn to use it. New rust coder will learn from it. Both win. |
This PR introduces the offical rust formatter
rustfmtto lyon.Changes
In the first commit, the
rustfmt.tomlconfiguration file is added.The style defined here is very open for discussion.
I just copied the configuration I use in my projects, so just tell me what you desire.
The second commit, formats the whole codebase.
The third commit adds a github action to check if the codebase was formatted correctly prior to commiting.
Purpose
Having a formatter is very valuable, since it creates consistency all over the codebase and the style is the same for every contributor.
I think it also lowers the bar for contribution, because a familiar code formatting makes the code base more approchable.
I would also consider this to be part of the 1.0 milestone effort, since it is related to rust standard practices.