Conversation
|
Big 👍. Selfishly, I would also like to wait until #189 and #194 have been merged before actually running the formatter – rebasing big PRs on a bunch of formatting changes is not my idea of a good time. I'm not worried about #182 because it's almost totally self-contained. But this PR is just the tooling, so it can be merged ASAP. As far as the choice of formatter: Spotless is new to me. If I'm reading this right, it doesn't do the formatting itself (à la Uncrustify), but rather, it's a sort of meta-formatter that invokes another formatting engine? Do you know what the implications of that are for running as part of CI? I.e., does Eclipse need to be installed on the CI machine? I ask because I'd really like to eventually wire this up as a GitHub Action check so that PRs which introduce formatting errors are flagged. As for the formatting itself, my preferences don't always align with the Eclipse defaults, but I'd much rather have some standard than none at all. No arguments here on that front. |
|
I wasn't aware of Uncrustify, until now. Honestly, I don't know how spotless works internally. In any case there's no eclipse necessary on the build machine. I think it's only the ruleset of the default eclipse formatter, which is applied. You can also use different rule sets or define your own. The Travis build in this PR already fails, because of the formatting errors. So, no need to adjust the GitHub Actions.
Fully agree! I don't care about which formatting rules are applied. |
Oh, would you look at that. I didn't actually bother to look at why the Travis build failed, because it's been dodgy for a while. I assumed it was an unrelated failure. Anyway, #189 replaces Travis outright with GitHub Actions, so its workflow does need to do the style check. It doesn't currently invoke |
|
I just tested That the build of this PR fails is intentional as the code is still unformatted. |
This adds spotless to the gradle build. The code can be formatted with
gradle spotlessApply. It uses the Eclipse default formatter, but VSCode integration is also available.If this is accepted, I will file a follow-up PR to do the formatting and manually check if everything has been formatted as intended. But this should probably be done after #189 and #194 has been merged.
This has been suggested by @MrDOS MrDOS#3 (review) and I had it also in my mind.