search

stm32-rs / stm32l0xx-hal

A hardware abstraction layer (HAL) for the STM32L0 series microcontrollers written in Rust
BSD Zero Clause License
96 stars 60 forks source link

Enforce rust fmt #107

Closed almusil closed 4 years ago

almusil commented 4 years ago

@hannobraun Can you please take a look?

dbrgn commented 4 years ago

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 commented 4 years ago

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

dbrgn commented 4 years ago

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 :slightly_smiling_face: 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.

hannobraun commented 4 years ago

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.
  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). 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 commented 4 years ago

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

@almusil, can you take a look?

almusil commented 4 years ago

@hannobraun Rebased, thanks

almusil commented 4 years ago

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 commented 4 years ago

Any update?

dbrgn commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

Thank you, @almusil!