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

Auto format to Google style with Spotless #317

Merged
merged 3 commits into from
Nov 28, 2019
Merged

Auto format to Google style with Spotless #317

merged 3 commits into from
Nov 28, 2019

Conversation

ches
Copy link
Member

@ches ches commented Nov 22, 2019

We state in the contributing guide that we conform to Google Java style, but actually we don't. This applies the Spotless Maven plugin which runs the Google formatter automatically.

The plugin can also take care of license headers, I didn't enable that yet but I can if you'd like.

This is obviously opinionated, I went ahead and sent this PR to open discussion because actually writing the change took 5 minutes, I won't be hurt if we don't use it. Also I thought that just after cutting 0.3 may be as opportune a time as it gets, hopefully not too many branches/PRs in flight that will suffer merge conflicts from a mass formatting application.

Having coding standards checks run automatically in the build adds a bit of friction for contributing. Automation though is arguably favorable to human reviewers remarking subjectively about such matters, for both parties. Measures can be taken to make the CI experience as pleasant as possible, suggesting how to fix infractions and resubmit.

Personally, I don't write Java everyday, so I don't have a tuned sense of what a lot of constructs should idiomatically "look like" and I appreciate it being answered for me, but I warmed to auto-formatting years ago so I'm biased.

As one point of precedent, the Beam project uses Spotless in their build. Of course in the Kubernetes ecosystem like Kubeflow, go fmt is ubiquitous and we mention it for the Feast Go SDK.

@feast-ci-bot
Copy link
Collaborator

Hi @ches. Thanks for your PR.

I'm waiting for a gojek member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@woop
Copy link
Member

woop commented Nov 22, 2019

/ok-to-test

@woop
Copy link
Member

woop commented Nov 22, 2019

We state in the contributing guide that we conform to Google Java style, but actually we don't. This applies the Spotless Maven plugin which runs the Google formatter automatically.

The plugin can also take care of license headers, I didn't enable that yet but I can if you'd like.

This is obviously opinionated, I went ahead and sent this PR to open discussion because actually writing the change took 5 minutes, I won't be hurt if we don't use it. Also I thought that just after cutting 0.3 may be as opportune a time as it gets, hopefully not too many branches/PRs in flight that will suffer merge conflicts from a mass formatting application.

Having coding standards checks run automatically in the build adds a bit of friction for contributing. Automation though is arguably favorable to human reviewers remarking subjectively about such matters, for both parties. Measures can be taken to make the CI experience as pleasant as possible, suggesting how to fix infractions and resubmit.

Personally, I don't write Java everyday, so I don't have a tuned sense of what a lot of constructs should idiomatically "look like" and I appreciate it being answered for me, but I warmed to auto-formatting years ago so I'm biased.

As one point of precedent, the Beam project uses Spotless in their build. Of course in the Kubernetes ecosystem like Kubeflow, go fmt is ubiquitous and we mention it for the Feast Go SDK.

Good points @ches

Another one to note is that we are using https://pypi.org/project/black/ for formatting Python code. I'm quite in favor of having these tools take over completely, especially during submission of PRs. I also think adding it for the license headers makes sense.

@woop
Copy link
Member

woop commented Nov 26, 2019

@ches if you can refresh this then I will approve it. Sorry for the delay.

@ches
Copy link
Member Author

ches commented Nov 27, 2019

No worries. Rebased and added the license header support for Java files.

Removing blank lines between existing headers and package declarations was not intentional, but I couldn't quickly figure out how to preserve them. A literal newline in the CDATA didn't work, Spotless is doing an equivalent of strip or trim somewhere I would guess.

@davidheryanto
Copy link
Collaborator

/retest

@woop
Copy link
Member

woop commented Nov 27, 2019

/lgtm

@feast-ci-bot feast-ci-bot removed the lgtm label Nov 28, 2019
@zhilingc
Copy link
Collaborator

/lgtm

@woop
Copy link
Member

woop commented Nov 28, 2019

/approve

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ches, woop

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@woop
Copy link
Member

woop commented Nov 28, 2019

/retest

We really need to fix these tests :(

@feast-ci-bot feast-ci-bot merged commit 5402b6e into feast-dev:master Nov 28, 2019
@ches ches deleted the spotless branch November 28, 2019 07:16
woop pushed a commit that referenced this pull request Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants