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
Files are not formatted with latest rustfmt #172
Comments
If you decide to routinely apply |
@bodymindarts rustfmt is constantly changing their rules. If you can produce a formatted version of this codebase which is idempotent under five consecutive versions of rustfmt I may consider this, but if not, it's just noise. |
@apoelstra It's nearing 1.0 so formatting should stay reasonably stable now. https://www.reddit.com/r/rust/comments/97cyfw/rustfmt_10rc/ |
Adding a I have rustfmt on save and it can be hard to commit changes that way :) I usually end up using vim (instead of nvim) to save the file without rustfmt to make the commit. |
Waiting for 1.0, adding a |
I'd be happy with a |
I don't think you can turn individual rules on / off so that they are ignored. You only have the option of setting the behaviour of the individual rules (as documented here). An empty Re: Githook
But for it to get executed individual developers would still have to execute
If we do that we'd have to at least document it. |
I ran It did miss some trailing commas, e.g. doing
( So I think at this point I'd be willing to accept a format-only PR, especially since several people here have their text editors set to run this on save. cc @TheBlueMatt what do you think? We could add |
I would want to propose one deviation from the default rustfmt setting: use_small_heuristics = "Off" Otherwise rustfmt arbitrarily breaks a line even if it doesn't reach the 100-character line limit because it "feels smaller". edit: fyi, this is my default rustfmt config: https://gist.github.com/stevenroose/468b01df07412b774a38a89453a82038 |
@stevenroose if you feel that something is wrong with rustfmt's defaults then it is much better to take it upstream than try to invent custom formatting. |
I think everyone agrees that If we are concerned about the commit history, we can have multiple commits targetting things file by file in a separate PR and finally create a separate PR with a single commit for cleaner history. |
I agree that |
concept ACK. But good luck doing this in practice. There are no "attributes to disable fmt for some parts of the code", you have to invoke I have tried running rustfmt on this codebase several times over the last year. Every time it makes a mess of things, and I am not even considered the ongoing noise it will produce; even if it is "stable" it still does things like splitting/unsplitting giant iterator chains into several lines based on a line length being changed by 1. |
From https://github.com/rust-lang/rustfmt#construction-of-config-options
Also https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#configuration-file-resolution Can be combined with https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#ignore
From https://github.com/rust-lang/rustfmt#tips
|
I wonder if anyone have built a CI tool that would enforce rustfmt only on files that are new or were already passing rustfmt previously. That seems like a nice way to keep the formatting improve with time, without a big switching event. Edit: Just asked on user forum. |
Amazing! Thanks @dpc |
So, the approach from One idea that I have that might be simpler and work for
prints the list of files that are not correctly formatted. One could check in If it changed for the better the author of the PR would have to go ahead and delete it from If it changed for the worse, the author of the PR would have to make it confirm and push the PR again. This way no big event is needed, and formatting should eventually get fixed everywhere, at which point the list can be removed. |
Perhaps we should consider introducing |
I like @dpc's suggested approach. We could start here if we initially had every single file excluded from format-checking. |
I'd vote somewhat strongly against rustfmt use. In general I find it makes files substantially harder to get context from - preferring (without a lot of config options to change it) to make files 2-3x longer than they should be, which makes it really hard to get context for what you're working on in some cases. We spent some time trying to use it in |
The problem with formatting is that everyone has their own preferences, habits and setups, text editors, IDEs, screen sizes, font sizes and so on and one can't make everyone happy. Some people like to line up multiple windows side by side horizontally, and like their code narrow, some have their terminal at the bottom, and one window above, and like their code wider. Rust settled on 4 spaces and breaking code more than less which makes it appear taller. Coming from C embedded/kernel background, I'm historically a 8-wide taber and I like my code wide. Too bad for me. I had to change my habits. Maybe I organize my work setup slightly differently now, maybe my font is different to counterbalance it, I can't really tell now. I guess I do use much more the Gnome's "snap to side border to maximize to half screen" feature routinely. Maybe I even structure my code somewhat differently. Anyway - I'm just used to it. For one time pain of altering my habits, usually any Rust project I jump into seem superficially familiar and ergonomic, and same goes for other people jumping into my code, I think. All in all it's a huge win comparing to things were in C/C++ land where every project was it's own little zoo, with stuff going more left, right, parenthesis going here, or there, and especially in C++ with a different subset of the language being blessed/cursed in every organization. |
Rust the project may have, but rustfmt has some material configurability for some things. eg it supports tabs instead of spaces just fine, or more spaces, or whatever. It also has some settings for how comically it makes your code unreadably tall, but not nearly enough to compensate for its tall-code goals. My complaint about rustfmt generating unreadable code isn't just about it looking bad to my eye, though, if it were just awkward indentation or breaks in places I don't like, I'd suck it up and move on. But making code impossibly tall has real, very non-zero cost. I've seen many times bugs be missed in review because of interaction with code 10 lines up or down in the file, sometimes 20. Suddenly you 2x the height of your file (rustfmt often does more than that) and instead of it being a question of making sure you have enough lines in your diff, its a question of scrolling through a file, and likely missing the bug anyway cause it isn't visible at the same time as the offending change. At least personally I generally find any flavor of code pretty readable, I'm just as comfortable reading validation.cpp in core as I am reading rest.cpp or old satoshi code - its all pretty different, but it all has some semblance of reason and none of it gets in my way for being able to scan the code to see what's going on where. Making code obscenely tall, on the other hand, makes things fall off screens and you miss critical context. Going and editing Core code (in one of its 10 flavors) or kernel code or any other code all is pretty easy, I can look around, see what's going on, and edit things. Nearly every time I try to jump in and edit a rustfmt-formatted project, I spend a bunch of time scrolling up and down to get a feel for what's going on, something I rarely have to do in any other context. Making less code fit on a terminal, or at least way less code fit on a terminal is about the only reason I'd ever have for thinking a formatter is garbage. |
In addition to Matt's general dislike for tall code, another very frustrating thing rustfmt does is auto-splitting lines when they exceed some maximum length, so that if you add (or remove) a single parameter from a function definition, this will change the line from 1 line to 10 (or vice-versa) completely obscuring the original diff. This, and its habit of ruining hand-formatted |
While I mostly agree with the sentiment of people against rustfmt in my experience it was occasionally useful to point out code that could've been written better by some other technique - splitting functions into smaller (max 15 lines is pretty good rule of thumb), assigning values to variables etc. I'd agree with making it a rule to prefer these techniques although it's hard to specify exactly. |
BTW -- I continue to be alright with "enabling" rustfmt on a set of zero files and trying to chip away at them all. It may be that some files (e.g. ones with lots of weirdly-formatted arrays) we'll never be able to rustfmt, but that's fine. On most files we have no big array literals and no long lines. |
31c4983 Fix IRC log record on gnusha.org (Dr Maxim Orlovsky) e1c8e13 Contributing: improving language and style (Dr Maxim Orlovsky) 45dbaa7 Contributing: remove derivation section (Dr Maxim Orlovsky) 313ac7d Contributing: improve formatting section (Dr Maxim Orlovsky) 78d1a82 Contributing guidelines (Dr Maxim Orlovsky) Pull request description: We've being discussing that a `CONTRIBUTING.md` guidelines are needed for some time and I took an effort to propose it. The main text is taken from `rust-lightning` version (with some additions), but it is extended on the topics we were discussing in multiple issues previously: - list of repository maintainers - standard derives - #555 - naming conventions (after Bitcoin Core) - MSRV and 2018 update #510 - code formatting with `rustfmt` #172 Each of these issue-related amendments to the basic (modified) version of rust-lightning contributing guidelines are made with a separate commit. ACKs for top commit: dr-orlovsky: Sorry for disturbing, @RCasatta @sanket1729, but this needs just a review of a single fixup commit in 31c4983 since your last reviews of this PR. We don't want to merge without your ACKs since you did a lot of commenting here before. RCasatta: ACK 31c4983 Kixunil: ACK 31c4983 apoelstra: ACK 31c4983 sanket1729: utACK 31c4983. Looks good Tree-SHA512: 1b15334a4e867070c0b33ba5fbdb55aea90d09879254673cf93c8bea069118b3543289f6f291e085bf2dd90715b913b783bcb9b025b9113079095cf14c220275
Should this be closed in favor of #788? |
Yeah, I think so. |
Is this still needed, I saw no difference running |
Change psbt APIs to take in secp parameter
I'd like to understand the reasoning behind not using rustfmt to format the code (it has been the norm on other rust projects I've worked on).
Incase its not intentional (or perhaps just not formatted with the latest rustfmt version) I have opened a PR #171
The text was updated successfully, but these errors were encountered: