Skip to content

Auto format to Google style with Spotless - #317

Merged
feast-ci-bot merged 3 commits into
feast-dev:masterfrom
ches:spotless
Nov 28, 2019
Merged

Auto format to Google style with Spotless#317
feast-ci-bot merged 3 commits into
feast-dev:masterfrom
ches:spotless

Conversation

@ches

@ches ches commented Nov 22, 2019

Copy link
Copy Markdown
Member

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
Copy Markdown
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.

Details

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

woop commented Nov 22, 2019

Copy link
Copy Markdown
Member

/ok-to-test

@woop

woop commented Nov 22, 2019

Copy link
Copy Markdown
Member

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

woop commented Nov 26, 2019

Copy link
Copy Markdown
Member

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

@ches

ches commented Nov 27, 2019

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Collaborator

/retest

@woop

woop commented Nov 27, 2019

Copy link
Copy Markdown
Member

/lgtm

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

Copy link
Copy Markdown
Collaborator

/lgtm

@woop

woop commented Nov 28, 2019

Copy link
Copy Markdown
Member

/approve

@feast-ci-bot

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

Details 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

woop commented Nov 28, 2019

Copy link
Copy Markdown
Member

/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