Skip to content
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

Ran cargo fmt with default options. #118

Merged
merged 6 commits into from
Nov 7, 2019

Conversation

jonathanpallant
Copy link
Member

We'll get fewer random style changes if the repo was actually formatted in the first place.

@hannobraun
Copy link
Contributor

hannobraun commented Oct 30, 2019

I welcome this pull request, as I think it's a good idea to have this conversation. But I'm against merging it and think we should go with another solution.

The party line is that automated formatting is a good thing and should be universally applied. In most discussions I've had on the topic, it was kind of implied that this is self-evidently obvious and arguing against this goes against the natural order ("it's supposed to end these discussions"). Needless to say, I disagree.

My argument boils down to this: Automated formatting can never be as good as thoughtful manual formatting, as the tool doesn't understand the code. It understands, and formats, the code on the syntax level, without regard for what any of it means. What it does is create consistency on the syntax level. I don't care about this, I care about readability. Consistency is a tool that should be applied, or purposefully not applied, in the service of readability.

The problem appears when syntactic consistency and readability are at odds. I've seen many examples of this, but the most egregious one is certainly the usage of svd2rust-generated APIs, which is pervasive in this repository. While automated formatting has its advantages, I believe those are outweighed, by a wide margin, by the disadvantages that come with the reduced readability (although the discussion in the other thread has shown that not everyone agrees it's actually less readable).

Please note that I'm not arguing against the specifics of the rustfmt style (although I honestly think that a lot of the things it does suck). I'm arguing against the concept of automated formatting, and I don't think my concerns can be fixed by any configuration of or improvements to the tool (not until we have AI that can understands code and the resources to run that AI on everyone's computer).


All that said, @jonathanpallant made a suggestion that alleviates some of my concerns, and as a result, I could learn to live with rustfmt. I'd like to note the irony though, that with a tool that's supposed to make my life easy by taking away part of my workload (the formatting), I now have to carefully format my code to prevent the tool from messing it up.


I believe we should do the following:

  • Leave this open for a bit to discuss.
  • Once discussion dies down, @nrf-rs/nrf52 should take a vote.
  • If we vote for automated formatting, this pull request should be amended to enforce this style in CI and be merged.
  • If we vote against, we should add a rustfmt configuration file to the repository that disabled all automated formatting.

Copy link
Contributor

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm vetoing this pull request until:

  • Another commit is added that enforces formatting in CI.
  • We have a majority vote by @nrf-rs/nrf52 in favor of automated formatting.

See my other comment for details.

Jonathan Pallant (42 Technology) added 2 commits October 30, 2019 10:22
@jonathanpallant
Copy link
Member Author

I've now split out the register writes so that the register/value pairing is more obvious.

Given that, does anyone think there is anything in this PR that makes things less readable? If so I'm happy to look at ways to resolve that, either through proposing some alternative formatting, or with a lint.

For me, having consistent whitespace is more than enough reason to merge this. I find inconsistencies in formatting really hard to deal with, even just browsing the source, never mind reviewing diffs.

For the avoidance of doubt, I'm happy with the next steps approach outlined above.

@jonathanpallant
Copy link
Member Author

FYI, I also prefer hard tabs from an accessability point of view, but I'm aware that's more controversial. The existence of the empty rustfmt file is to bypass by system default setting for hard tabs.

@hannobraun
Copy link
Contributor

I've now split out the register writes so that the register/value pairing is more obvious.

Thanks! That looks much better.

Given that, does anyone think there is anything in this PR that makes things less readable? If so I'm happy to look at ways to resolve that, either through proposing some alternative formatting, or with a lint.

There are still some things that I'd prefer to be different, but I think those are matters of taste and not worth following up on at this point.

@jonathanpallant
Copy link
Member Author

@nrf-rs/all - what do we think on this one?

Copy link
Member

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jonathanpallant
Copy link
Member Author

jonathanpallant commented Nov 4, 2019

Review majority from nrf-hal/nrf52-hal team. 8 members, 5 or more required for majority.

  • chocol4te
  • hannobraun
  • jamesmunns
  • jonathanpallant
  • jscarrott
  • korken89
  • wez
  • Yatekii

Plus therealprof, who doesn't appear in the nrf52-hal members list and hence doesn't technically meet hannobraun's rules on dropping his veto.

@fmckeogh
Copy link
Contributor

fmckeogh commented Nov 5, 2019

I've looked at all the changes, I can certainly understand the arguments against fmt (I personally find the formatted nrf52-hal-common/src/gpio.rs less readable), but I'm absolutely in favour of automated formatting. For me consistency and automation is much more important than the improved quality of manual formatting. Not that I'm really enough of a contributor to be adding meaningfully to this discussion ;)

@Yatekii
Copy link
Contributor

Yatekii commented Nov 5, 2019

I personally don't like autoformatting and on top of that I don't like rustfmt's default at quite some places (e.g stupid formatting of imports, first argument on same line as opening bracket and second argument on next, and many more). Most of it's better style choices are still unstable. I am not sure if they'll ever be stable, but if they do get stable and we can configure it in sane fashion, i am all in for a standard style.
Until then I won't vote in favor but I will also not roadblock this. We had this discussion once already and there was many pros and cons. Doing it over and over again is not productive either.

@korken89
Copy link
Contributor

korken89 commented Nov 6, 2019

When it comes to automatic formatting I was strongly against it, but the consistency got me to feel like home in any repository which made me very strongly for it.

@Yatekii
Copy link
Contributor

Yatekii commented Nov 6, 2019

I guess we can merge it then. @wez and @jscarrott don't really chime in here :)

I just wish that we use sane defaults once they come of age and are stabilized =) Because many defaults are simply not sane in rustfmt right now.

@hannobraun
Copy link
Contributor

Yeah, it's been a week and of those who stated their opinion, we have a majority in favor of merging (no matter which specific team is supposed to vote here).

The only thing left before merging is to figure out why Travis is failing.

@hannobraun hannobraun dismissed their stale review November 6, 2019 19:11

Requested changes have been made and team has voted in favor.

@jonathanpallant
Copy link
Member Author

Ta da.

@jonathanpallant jonathanpallant merged commit ee6d95c into nrf-rs:master Nov 7, 2019
@jonathanpallant jonathanpallant deleted the run_cargo_fmt branch November 7, 2019 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants