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

Add formatter #199

Merged
merged 1 commit into from
Jan 20, 2021
Merged

Add formatter #199

merged 1 commit into from
Jan 20, 2021

Conversation

fwolter
Copy link
Collaborator

@fwolter fwolter commented Jan 13, 2021

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.

@MrDOS
Copy link
Contributor

MrDOS commented Jan 15, 2021

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
Copy link
Collaborator Author

fwolter commented Jan 16, 2021

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
Copy link
Contributor

MrDOS commented Jan 16, 2021

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
Copy link
Collaborator Author

fwolter commented Jan 16, 2021

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.

@MrDOS MrDOS merged commit 730a630 into NeuronRobotics:master Jan 20, 2021
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.

2 participants