Enforce rust fmt #107

almusil · 2020-05-27T11:51:28Z

@hannobraun Can you please take a look?

dbrgn · 2020-05-27T12:15:39Z

I'm definitely in favor of using and enforcing rustfmt everywhere.

Not sure whether this should be a single reformatting commit though. Is there any advantage in having multiple commits?

almusil · 2020-05-27T12:21:16Z

IMO it is better to keep the history what was actually done. But I don't mind squashing it into single commit.

dbrgn · 2020-05-27T12:31:52Z

I'm a big fan of keeping the history clean, but in "git log FILE" and "git blame FILE" both approaches will show up as a single commit 🙂 In contrast, I tend to find a single commit easier to "parse" in the commit history than 17. (The CI change makes sense as a separate commit though.)

In any case, no strong opinons on that. I do think enforcing rustfmt is a good idea though, it completely eliminates the opinionated "could you please format this part of the code in a different way" from PR reviews.

dbrgn · 2020-05-27T12:33:29Z

src/pwr.rs

-                    // Use MSI as clock source after wake-up
-                    w.stopwuck().clear_bit(),
+                // Use MSI as clock source after wake-up
+                {


This one looks a bit weird... Since when does rustfmt put opening curly braces on its their own line? Might be a mess-up due to the comment...

It probably is. I will rearrange the comment so it does not look that bad. Good catch thanks

almusil · 2020-05-27T12:40:36Z

src/pwr.rs


        // Configure Stop mode
-        self.pwr.0.cr.modify(|_, w|
+        self.pwr.0.cr.modify(|_, w| {


I see that I forgot to change this one. On it

hannobraun · 2020-05-27T13:12:20Z

Thank you, @almusil and @dbrgn! The formatting changes look good to me (well, as good as it gets with rustfmt'ed code, at least :-) ).

I see two minor problems with the last commit:

I think formatting should not be enforced on nightly. I think that has lead to problems in the past in other projects, but I don't remember clearly why that was. Probably, nightly rustfmt tends to make different formatting choices than stable/beta, which means it's going to fail all the time anyway.
It would be great if there was a script that was called by CI, that I could also run locally to figure out whether CI was likely to pass (example of this in another HAL). Otherwise, people are going to submit unformated code and won't even know right away, because CI takes a while to run and GitHub won't notify the PR author if it fails. This is going to lead to a lot of unnecessary back-and-forth.

I think it's fine to merge as-is though, and follow up with additional improvements later, as necessary. Especially point 2 is a bit of a can of worms of worms, due to the use of the build Matrix on CI (that other HAL I linked doesn't use the build Matrix for that reason).

@arkorobotics What do you think? I'm inclined to merge this, to get the formatting discussion over with.

hannobraun · 2020-05-27T13:29:09Z

I've just introduced a conflict by merging #101.

@almusil, can you take a look?

almusil · 2020-05-27T13:52:00Z

@hannobraun Rebased, thanks

almusil · 2020-05-27T13:53:59Z

Thank you, @almusil and @dbrgn! The formatting changes look good to me (well, as good as it gets with rustfmt'ed code, at least :-) ).

I see two minor problems with the last commit:
1. I think formatting should not be enforced on nightly. I think that has lead to problems in the past in other projects, but I don't remember clearly why that was. Probably, nightly rustfmt tends to make different formatting choices than stable/beta, which means it's going to fail all the time anyway.

Rust fmt might enforce different rules on nightly and also fmt is not always available there. So it makes sense to skip fmt on nightly.

2. It would be great if there was a script that was called by CI, that I could also run locally to figure out whether CI was likely to pass ([example of this in another HAL](https://github.com/lpc-rs/lpc8xx-hal/blob/master/scripts/build.sh)). Otherwise, people are going to submit unformated code and won't even know right away, because CI takes a while to run and GitHub won't notify the PR author if it fails. This is going to lead to a lot of unnecessary back-and-forth.

This should not be that hard I will create a issue for that and assign myself. Once I have some more spare time I'll look into this.

I think it's fine to merge as-is though, and follow up with additional improvements later, as necessary. Especially point 2 is a bit of a can of worms of worms, due to the use of the build Matrix on CI (that other HAL I linked doesn't use the build Matrix for that reason).

@arkorobotics What do you think? I'm inclined to merge this, to get the formatting discussion over with.

almusil · 2020-06-07T13:21:16Z

Any update?

dbrgn · 2020-06-07T20:24:20Z

There's a new conflict, maybe you can resolve that?

Rust fmt might enforce different rules on nightly and also fmt is not always available there. So it makes sense to skip fmt on nightly.

I think this is still missing as well, right? Or would that be part of #109?

almusil · 2020-06-08T07:30:48Z

There's a new conflict, maybe you can resolve that?

Done

Rust fmt might enforce different rules on nightly and also fmt is not always available there. So it makes sense to skip fmt on nightly.

I think this is still missing as well, right? Or would that be part of #109?

It was meant to be part of #109 but I can probably do it in this patch. Only thing that might make it harder are the conflicts so if it's not vital I would rather do it in follow up.

hannobraun · 2020-06-08T08:05:02Z

Nobody has complained, and this has been open long enough. I'm merging.

Thank you, @almusil!

almusil force-pushed the enforce_rust_fmt branch from db5f17d to ab7c448 Compare May 27, 2020 11:55

dbrgn reviewed May 27, 2020

View reviewed changes

almusil commented May 27, 2020

View reviewed changes

almusil force-pushed the enforce_rust_fmt branch from ab7c448 to 98f901f Compare May 27, 2020 12:42

hannobraun added the status: waiting for review label May 27, 2020

hannobraun added the status: waiting on author label May 27, 2020

almusil force-pushed the enforce_rust_fmt branch from 98f901f to 341a99b Compare May 27, 2020 13:51

hannobraun removed the status: waiting on author label May 28, 2020

almusil added 13 commits June 8, 2020 09:25

adc: Reformat with cargo fmt

fde3ea1

aes: Reformat with cargo fmt

eec5bd5

dma: Reformat with cargo fmt

b2488d9

exti: Reformat with cargo fmt

17f246a

flash: Reformat with cargo fmt

da746d0

i2c: Reformat with cargo fmt

c341ddf

lptim: Reformat with cargo fmt

5ce58d6

pwr: Reformat with cargo fmt

1b1a2dc

rcc: Reformat with cargo fmt

8474a5a

rng: Reformat with cargo fmt

dc1881e

rtc: Reformat with cargo fmt

d47a585

serial: Reformat with cargo fmt

d6f996a

time: Reformat with cargo fmt

cc263e5

almusil added 5 commits June 8, 2020 09:26

timer: Reformat with cargo fmt

c7c2c7e

usb: Reformat with cargo fmt

dcb18b0

examples: Reformat with cargo fmt

82f25be

lib: Reformat with cargo fmt

936c9d8

automation: Enforce cargo fmt in CI

5a744a4

almusil force-pushed the enforce_rust_fmt branch from 341a99b to 5a744a4 Compare June 8, 2020 07:27

hannobraun removed the status: waiting for review label Jun 8, 2020

hannobraun merged commit 489685c into stm32-rs:master Jun 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce rust fmt #107

Enforce rust fmt #107

almusil commented May 27, 2020

dbrgn commented May 27, 2020

almusil commented May 27, 2020

dbrgn commented May 27, 2020

dbrgn May 27, 2020

almusil May 27, 2020

almusil May 27, 2020

hannobraun commented May 27, 2020

hannobraun commented May 27, 2020

almusil commented May 27, 2020

almusil commented May 27, 2020

almusil commented Jun 7, 2020

dbrgn commented Jun 7, 2020 •

edited

Loading

almusil commented Jun 8, 2020

hannobraun commented Jun 8, 2020

Enforce rust fmt #107

Enforce rust fmt #107

Conversation

almusil commented May 27, 2020

dbrgn commented May 27, 2020

almusil commented May 27, 2020

dbrgn commented May 27, 2020

dbrgn May 27, 2020

Choose a reason for hiding this comment

almusil May 27, 2020

Choose a reason for hiding this comment

almusil May 27, 2020

Choose a reason for hiding this comment

hannobraun commented May 27, 2020

hannobraun commented May 27, 2020

almusil commented May 27, 2020

almusil commented May 27, 2020

almusil commented Jun 7, 2020

dbrgn commented Jun 7, 2020 • edited Loading

almusil commented Jun 8, 2020

hannobraun commented Jun 8, 2020

dbrgn commented Jun 7, 2020 •

edited

Loading