NeuronRobotics / nrjavaserial

A Java Serial Port system. This is a fork of the RXTX project that uses in jar loading of the native code.
Other
344 stars 143 forks source link

Add formatter #199

Closed fwolter closed 3 years ago

fwolter commented 3 years ago

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 https://github.com/MrDOS/nrjavaserial/pull/3#pullrequestreview-566800485 and I had it also in my mind.

MrDOS commented 3 years ago

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.

fwolter commented 3 years ago

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.

but I'd much rather have some standard than none at all

Fully agree! I don't care about which formatting rules are applied.

MrDOS commented 3 years ago

The Travis build in this PR already fails, because of the formatting errors.

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 gradle check, just gradle build, ~so it will need to be changed. I'll update that PR accordingly.~ I misunderstood what gradle build does – it's equivalent to gradle assemble + gradle check, so that PR is fine as it stands.

fwolter commented 3 years ago

I just tested gradle build. It invokes the check task already. I think there's no further adjustment necessary.

That the build of this PR fails is intentional as the code is still unformatted.