Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Add rustfmt (attempt) #16

Closed
sunjay opened this issue Apr 6, 2019 · 0 comments
Closed

Add rustfmt (attempt) #16

sunjay opened this issue Apr 6, 2019 · 0 comments
Milestone

Comments

@sunjay
Copy link
Contributor

sunjay commented Apr 6, 2019

I am opening this issue to document my attempt at adding rustfmt to our codebase. We really want a tool like rustfmt because it can help us get consistency across our codebase without having to ask everyone to make lots of small nitpicky changes in PRs.

I actually evaluate using rustfmt every few months because I really want it in all my projects, not just this one. The reason I was cautious against adding it in #13 is because rustfmt is still a really new tool. It's very actively developed and it changes all the time. That means that it still doesn't always format code very well. This can end up hindering us more than it helps, so I always try to run the tool first before I add it permanently.

It's come quite far, but I don't think rustfmt is mature enough for us to start using just yet. There are a lot of issues that still need to be resolved. For example, this one shows that rustfmt actually makes method chains (something very common in Rust) look worse. We could workaround this by adding #[rustfmt::skip] attributes everywhere, but doing that kind of defeats the purpose of having a formatter and ends up making our lives harder than they need to be.

I tried running rustfmt on our code and went through every configuration option in detail until I settled on this configuration:

max_width = 120
imports_layout = "HorizontalVertical"
merge_imports = true
match_block_trailing_comma = true
newline_style = "Unix"
reorder_impl_items = true
use_field_init_shorthand = true
use_try_shorthand = true
overflow_delimited_expr = true
unstable_features = true

Even with all of this, I still wasn't able to get even a satisfactory level of formatting in our codebase. The bugs in rustfmt are very apparent. They end up making the code look worse instead of better.

In addition to the bad aesthetics that rustfmt produces, another big issue is that many of the options I configured are actually unstable. That means that we have to use the nightly compiler to run rustfmt (not a huge deal) and also means that the behaviour of these options can change at any time (a very big deal). We don't want our build to break all the time because option defaults are changing or because options have been removed/renamed.

Given that rustfmt is currently in a state where it will end up inconveniencing us and where we have to use unstable options in order to run it, I elect that we do not use rustfmt at this time.

It's being very actively developed, so we can re-evaluate it in a few months. I don't think it makes sense to add it right now. We only have a little bit of code and we can keep policing it ourselves for the timebeing.

I'm going to open and then close this issue. It's only meant to be documentation of my attempt. We can open another issue to deal with this in a few months when we actually find that we really need the tool.

cc @ProtoArt/core-team

@sunjay sunjay closed this as completed Apr 6, 2019
@sunjay sunjay added this to the MVP - 0.1.0 milestone Jun 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant