Remove rustfmt formatting requirement from project#2127
Remove rustfmt formatting requirement from project#2127stevenroose wants to merge 4 commits intorust-bitcoin:masterfrom
Conversation
|
Awesome, during review, we can all get in the habit of writing "please fix formatting here to be some suitable layout, although we choose to define suitable as however each individual conrtibutor thinks the code should be layed out, oh you did that already - your layout sucks, oh I said that last week on an almost-identical change (thanks Steven)" |
junderw
left a comment
There was a problem hiding this comment.
There are enough voices of discontent with rustfmt throughout the Rust ecosystem.
I am actually curious to see examples of this if you have any.
I think formatting matters.
Then replace it with another tool that checks for your formatting... otherwise this is just increasing the cognitive load of everyone who reviews code for this repository and opens up each PR for endless bike-shedding.
Concept NACK.
Which is why I think plain old vanilla rustfmt without any customization is the best route to go. But that's my 2 sats. |
|
Perhaps we can split this problem into two parts
|
|
FWIW. I think code formatted with |
Me too. Just kidding. I have literally never looked at any code output by rustfmt and thought "wow that's ugly and or hard to read." As long as there's a consistent format and people agree upon it, and CI can check PRs for compliance, I don't care what it is. Some people argue that this "agree upon it" should be community wide. Some say it should be project wide... I think project wide is fine, but having a set of rules that is checked in CI is important to take away a large source of bike shedding on each issue. |
vincenzopalazzo
left a comment
There was a problem hiding this comment.
So, we should not format the code anymore? or we should use the cargo fmt --all without nightly?
Yeah this definitely seems to be a huge problem in other projects that don't use code formatters.. Like bitcoin core or rust-lightning.. And I'm obviously not against having some code formatting rules, like tabs/spaces, organizing of use statements, newlines around functions and if-blocks, basic things like that. Things that are actually important to be consistent and are generally just opinionated but can't really make a big difference in readability. If there is a tool that checks/fixes those things, great. That's what gofmt does for Go. But rustfmt isn't a format fixer, it's a canonical formatter, which will not just fix a few basic rules, it will entirely disregard everything you wrote and output it in a canonical format. |
Also it doesn't seem like the current formatting rules were added because this ever was a problem, but because one random dude (who's not even a contributor) wondered why we were not doing it. And then Maxim adding a formatting section to our |
This is the most recent one I came across and it made me decide to finally open this MR.
I'd rather go the Maxim way and fork the project in that case. |
Sure, write your code that way. Probably you won't because it would be insane to hit enter so many times as a human. But sure if you like it that way, write it like a normal human and then apply a formatter to your own code. Unfortunately I don't think there's a tool that can do that. |
Git can actually fix formatting and white space https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration. See the section "Formatting and Whitespace". |
|
We're not gonna accept this because there are too many strong views on the other side, but FWIW I totally agree wih @stevenroose. A bit of history though:
Eventually we moved to a scheme where we would format individual modules that nobody cared about using rustfmt, and we still had liberal This got mired because rustfmt's output was so bad that we couldn't format any more modules, until somebody found out that the nightly rustfmt lets you disable a lot more stuff. This got us from "impossibly bad" to "tolerably bad" so we could move forward. At the time, all developers except for @stevenroose were willing to download and run daily-replaced binary blobs from
I'm not going to accuse you of gaslighting, but this is an insane point of view ;). Right now there are 59 instances in this codebase of
There is no other tool in the Rust ecosystem. In a recent thread somebody found one that was somehow even worse than rustfmt in terms of mangling code in impossible-to-override ways. Somehow this does not even crack the top 5 problems with the Rust ecosystem. |
|
While I strongly agree rustfmt absolutely sucks, I think rust-lightning, too, is gonna go rustfmt. Having review comments around formatting is just too much of a roadblock to contributors to remain practical. We haven't figured out exactly what we'll do yet, and I personally refuse to run rustfmt binary blobs on my system in my general workflow (or use any rustup garbage except in rare cases). Sadly this means my workflow for contributing to rustfmt projects is to write code, then use CI to fix formatting. Is there no way to RUSTC_BOOTSTRAP on stable rustfmt here? |
What does this mean? If you just mean "use stable rustfmt", then personally I do think there's a path forward where we delete each line individually from our |
|
I meant is there a way to "use nightly rustfmt" on a stable rustfmt binary (similar to what you can do with some rustc/cargo features with RUSTC_BOOTSTRAP). |
Yes. All you need is BTW. We have twice as many Rust LoC in Fedimnt, contributors coming and going and of various level of experience. Stuff being ripped apart, rewritten furiously all the time. No one ever even mentions formatting. No one. For all intents and purposes formatting is a solved problem and we don't even think about it. Same in majority of Rust Open Source projects I contributed to. The ones where I remember that wasn't the case it was always some disagreeable opinionated author with strong opinions from previous work with C and C++. I wonder if a single person complaining about rustfmt formatting isn't a previous C/C++ developer. Please let me know if that's the case - it's just a theory, but I think that they generally overfit their neural network staring at C++ where every |
@dpc the goal is that you don't need a nightly rustfmt to be installed at all. Ideally you can just tell a stable rustfmt "pretend you're a nightly".
Lol. I'm curious about this as well. (For my part, yes, I am a previous C and C++ developer.) |
It requires the nightly (or dev) release channel. It detects it with the |
Wait, so does that mean I can do |
I tried it, but I couldn't get it to work... |
I know this was tongue in cheek, but just in case someone thought I was being superfluous, I'm being 100% serious. Code formatting just really isn't that important to me. I can read it just fine. But then, of course, I don't come from a C/C++ background... I come from a higher level language background and all the projects I've worked on so far always had formatters as a given. |
FWIW I came from C, found out about code formatters in Go then worked in Rust. In the Linux kernel the "solution" to this problem is what made me be so opinionated about using a formatter
So basically its just a big mess and as long as the formating tool is bearable I think its a big win. FTR I routinely change my code (variable/paramater names, static strings etc.) so that it formats in a not-so-ugly way, this is a fail and I don't think one should ever need to do this. It is just possible with ones own code, it is impossible to PR with "change variable name so |
Can be said about any code. "Rust code formatted by apoelstra makes me unhappy and not want to read the code anymore at all". 🤣 "Too dense, hard to read, goes too much horizontally, so when working with split screen, I have to use a scroll bar and can't see everything, especially painful in split column diff view." Before automatic and enforced formatting has become popular, every job, every team, every project was another set of pointless, arbitrary rules that 90% at the time dislike and thus constantly argue about, devolving into petty power struggles for nerds. Nowadays I'm happy people can hate on how |
|
Ok, this PR has successfully derailed my morning and I hazard a guess its done the same to a few others. All I've gotten out of this is:
(Purposely not using mentions so as not to waste even more of peoples time.) I'm closing this as a total waste of everyones day. |
I don't think this discussion has been a total waste and it feels premature to close this even if these changes will not be accepted as is. Using rust fmt nightly can be an annoyance to peoples (rather opinionated) workflow. This project is different from most others in the desire to reduce third party dependencies, and arbitrary rustfmt binaries is such a dependency. Closing this PR and "wishes the whole thing would go away" seems silly. Although the branch title of this PR seems silly also and not very serious. |
I don't think closing this PR is "wishing this would all go away" (though Tobin did say that this is how he feels :)). I agree that the discussion has been useful (at least, to collect a ton of spread-out discussion in one place) but I also think it's pretty-much played out. |
Thanks @apoelstra.
If you have a workflow where disabling a tool like rustfmt for a certain project is a non-trivial task, I would seriously doubt the quality of the code these people contribute. Seriously it's like complaining a project uses spaces because your editor only does tabs.
It's not even that I'm unwilling. rustup doesn't work on Guix, so I would have to manually build rustc versions from Guix's latest to stable, then build rustc nightly and then build rustfmt with it.
As it currently stands, even during my sabbatical most MRs here get merged before I have a chance to look at them, so I won't want to try that. (Though commenting on already-merged MR might be a statement against the short review times people are allowed here 🤔 ) I think I already make a statement by distancing myself in my own MRs from the formatting changes introduced by rustfmt.
I'm confused which single person you're talking about? This thread alone has several people complaining about it.. I personally barely wrote any C or C++ in my life. I do agree that Rust's type system slightly reduces the importance of reviewability and readability of code, but I would argue that the potential impact of serious issues in a lib like rust-bitcoin would make that more of a priority compared to other projects where the absolute worst that can happen is "the program doesn't work".
@junderw maybe you have a ton of vertical lines? I have a lot as well myself, 72 or something, but I've had colleagues with worse eye-sight that only had less than 50 or 40 vertical lines and like that you can very often not get the function you're working on in your screen. I would actually recommend anyone that thinks rustfmt is great to try develop using only 40 lines of vertical space and see how easy you find it to keep function context while writing code. Especially people with mice or that don't use something with efficient key bindings like vim have to do a lot of hand-switching in that case. (I personally have non of these problems, but I have worked with people that have complained about these. I feel like the people that struggle with this wouldn't easily speak out about this as they risk being "that one guy that wants to get rid of rustfmt" in their own project.)
I am not going to cite each individual section of your comment, but wow, I know many developers aren't your average Joe, but I can't help but be amazed with the level of OCD if so many of them can't stand a piece of code have a slightly different style than the next one.
I think it's kinda unfortunate to close this conversation after only a single day as it looks that there are quite some people unhappy with rustfmt in general and even those that are strongly in favor of keeping rustfmt don't really seem to like the way it reformats their code, but seem to just accept any standardized format over actually well-formatted code. |
|
Re-opening since my closing seems to have bothered people (I've unsubscribed from notifications on this PR :) |
There's a price to pay for deviating from mainstream, and you can only pile so many deviations before it falls apart. FOSS community is good at accommodating preferences and experimentation, but ultimately if Guix doesn't have a solution to dealing with rather basic and common requirement, than it's not a practical OS to use. I would expect Guix to be able to roughly do the same thing as Nix. A dev system is for writing code that will be reviewed anyway and recompiled multiple times. Bootstrapping from 15 bytes base, or whatever was the latest record, is cool and maybe even useful in some use cases, but not very important for working on the code. And there are also VMs. Qubes users run everything in a VM and they seem happy.
I'm using fish, tmux, helix in a 40 line terminal for about everything. That's how my terminal looks ATM. Sometimes, when I feel extra grandpa, I up the font size on time, and it goes to 36 lines. And that's a primitive setup. Kids nowadays have GUIs, folding for code and comments, minimaps and probably conveniences that I don't even know about. Seems like anyone should be able to figure out something to make it workable.
People have preferences. Some, especially after working mostly by themselves or in tiny groups (which was not uncommon before the Internet collaboration became a norm) for decades will have preferences much stronger than
|
It's just an extra inconvenience. I'm sure everyone is able to do this but they'll probably have to do a bit of digging to remember how they set up rustfmt in the first place. Tabs vs spaces is similar but in some languages there is no universal standard so this is a pretty accessible and standard thing to have to change.
I also had a bit of trouble parsing @dpc's sentence. I believe he meant: if anybody here complaining about rustfmt isn't a C or C++ developer, please raise your hand. It sounds like that's you. |
d391ada ci: nightly rustfmt PR scheduled/manual (Einherjar) Pull request description: ## Summary - Removes nightly `rustfmt` checks from CI and `CONTRIBUTING.md`. Superseds #2127 - Adds automated nightly `rustfmt` PR creations manual and every Sunday 00:00 UTC. Closes #2128. Closes #1712. ## Details The new `.github/workflows/rustfmt.yml` action does the following: 1. Checkouts the repo 1. Install the nightly Rust toolchain with `rustfmt` component 1. Run `cargo +nightly fmt` which format all the codebase with the nightly Rust toolchain writing the changes 1. Add the current date in `YYYY-MM-DD` format as an GH action ENV var 1. If changes are detected, create a PR with the following details: - Title: Automated nightly rustfmt (`DATE`) - Body: Automated nightly `rustfmt` changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action - Commit message: `DATE` automated rustfmt nightly - label: `rustfmt` I did a test run (with `workflow_dispatch` and some minor changes to trigger the PR) in my fork. Check how the PR looks in https://github.com/realeinherjar/rust-bitcoin/pull/5 Of course, all of these things can be changed. They are all suggestions... ## Security Disclaimer We are only introducing a new action, which is quite battle-checked, [`peter-evans/create-pull-request`](https://github.com/peter-evans/create-pull-request) with ~2k stars and proper maintained. ACKs for top commit: apoelstra: ACK d391ada tcharding: ACK d391ada Tree-SHA512: c8728d03467913005fc92fefd4150a8041a495b0ec08b3a3397a05f381a59ef904e8afe6182509fd295ad1b23875b43206a2f1238a253542da54420e4805ab6f
|
Should this be closed since #2135 was merged? |
|
We resolved this already with the weekly fmt bot. |

There are enough voices of discontent with rustfmt throughout the Rust ecosystem. Personally, reading rustfmt-formatted Rust code makes me unhappy and not want to read the code anymore at all. rustfmt generally sucks at formatting code in a sensible way, has absolutely no respect for the developer's judgement on readability, nor for vertical space. For me, it almost always makes code less easy to read and manage. Not even talking about adding nightly Rust to our required development stack.
Anyway, just my two bits. Had to throw them in here. I think formatting matters. It's an important tool to make code understandable, glancable and manageable.
Not wanting to repeat,
Format checking was added to this project without any motivation given whatsoever, kinda as a random side-effect of adding a CONTRIBUTING section. The issues people seem to be scared of in this thread have never ever been a problem in this project before.
I'm obviously not against basic formatting rules and if there was a tool that could enforce them, that'd be great; unfortunately it doesn't exist for Rust code.