You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2022/04/20 04:14:20 UTC
[GitHub] [thrift] Jimexist opened a new pull request, #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Jimexist opened a new pull request, #2581:
URL: https://github.com/apache/thrift/pull/2581
<!-- Explain the changes in the pull request below: -->
use spotless plugin and google-java-format to enforce a consistent code format
- https://github.com/diffplug/spotless
- https://github.com/google/google-java-format
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? (not required for trivial changes)
- [ ] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [ ] Did you squash your changes to a single commit? (not required, but preferred)
- [ ] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ctubbsii commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1119012535
I resolved the merge conflicts. Just waiting on CI before merging this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ctubbsii commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1119250880
@Jimexist I've verified everything is working for me locally now... but I'd like to see it pass in CI. I had to modify the spotless plugin configuration to only target the src/ directory, because `gradle build` was failing for me locally on the spotlessCheck when it was looking at the generated Java files under the build/ directory. I don't think I saw this fail in the GitHub Actions, though, and I'm not sure why. Running the check before any of the files were generated always worked, though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jimexist commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
Jimexist commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1118068179
> As an example, Accumulo [does this](https://github.com/apache/accumulo/blob/main/.github/workflows/maven.yaml#L59) to verify the format during CI. Note the `-DskipFormat -DverifyFormat` (feel free to consult the pom.xml to see how that works, but I think these two properties are enough to illustrate my point).
@ctubbsii thanks for the suggestion, i added one step for checking within the GitHub workflow action
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ctubbsii commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1117740256
> Yes sorry I wasn't clear in the beginning. The spotless check is a standalone step already included into gradle build - but you can also run it on its own. It does the check on Java lib code that's checked into git.
I understand that spotlessCheck is included into the gradle build. However, I'm suggesting that for it to be useful as a CI check, it needs to be run on its own, outside the normal gradle build.
In development, we want: `gradle build` to execute `spotlessApply` so it formats all the code. After that, it can run `spotlessCheck` to make sure everything was formatted, but it's kind of redundant, because everything should have been formatted by the previous `spotlessApply` step. We don't expect the build to fail based on formatting here. But, the developer should check in any changed files, to include in their pull request.
In the pull request CI checks, we want something different. It is not sufficient to simply run `gradle build`, which will run `spotlessApply` to format all the source code. We actually want to run `spotlessCheck` **without** running `spotlessApply`, because we're not trying to *format* the source code during CI... we're trying to *detect unformatted* source code.
So, we need to run `spotlessCheck` on its own, without running `gradle build`, because we want to avoid running `spotlessApply` before running `spotlessCheck`. If we do `gradle build`, the check will always pass, and we will fail to detect that some files in the PR weren't formatted.
Imagine that there is unformatted code in a PR. We want a CI check to ensure code in PRs are formatted already, so we don't merge unformatted code.
As I understand it, `gradle build` will, among other things, run `spotlessApply`, then `spotlessCheck`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ctubbsii commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1117743035
As an example, Accumulo [does this](https://github.com/apache/accumulo/blob/main/.github/workflows/maven.yaml#L59) to verify the format during CI. Note the `-DskipFormat -DverifyFormat` (feel free to consult the pom.xml to see how that works, but I think these two properties are enough to illustrate my point).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ctubbsii merged pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
ctubbsii merged PR #2581:
URL: https://github.com/apache/thrift/pull/2581
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jimexist commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
Jimexist commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1104608461
> Are the formatting changes already checked in, or is that expected to be a follow-on?
thanks @ctubbsii the actual commit is https://github.com/apache/thrift/pull/2581/commits/f840a08d920a769602081f0b931f065daddf3877 and it's done using `gradle spotlessApply`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jimexist commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
Jimexist commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1116100925
> > The specific command is gradle spotlessCheck for checking which compliments gradle spotlessApply which applies the format, the former is by default included into gradle build and will do nothing is format is correct otherwise will fail gradle build
>
>
>
> What I'm suggesting is that the check should verify that the code that is checked into the repo is formatted, not the code that is the result of the build is formatted. That implies that spotlessCheck should run as an independent check, without running spotlessApply.
Yes sorry I wasn't clear in the beginning. The spotless check is a standalone step already included into gradle build - but you can also run it on its own. It does the check on Java lib code that's checked into git.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jimexist commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
Jimexist commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1118073799
>
the step in effective is https://github.com/apache/thrift/runs/6299416812?check_suite_focus=true
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jimexist commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
Jimexist commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1119259432
> I don't think I saw this fail in the GitHub Actions, though, and I'm not sure why.
because `generateJava` is the step that actually generates the java files from thrift, but it's run during `make check` not `make`.
i shall later add `make check` into the GitHub actions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jimexist commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
Jimexist commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1113879957
After the rebase (and reformat) this seems fixed now. @ctubbsii you can verify by `git checkout master -- lib/java/src/main` and then run again `gradle spotlessApply` to see that all code changes are indeed auto-format.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ctubbsii commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1119259285
Passing in GH Actions is sufficient for me, since I've verified `gradle build` locally, and Travis and Appveyor seem really flaky and unreliable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jimexist commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
Jimexist commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1116069572
Yes the format is fast enough and it is included by default into gradle build so I think it's already enabled now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ctubbsii commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1116068062
> > After the rebase (and reformat) this seems fixed now. @ctubbsii you can verify by `git checkout master -- lib/java/src/main` and then run again `gradle spotlessApply` to see that all code changes are indeed auto-format.
>
> well actually that was a lie - for only one of the file i indeed need to remove the wildcard import as to remove ambiguity of `String` generated by thrift and `java.lang.String`
If the check is fast enough, could add a task to format during CI, and fail the CI check if it results in any changes to tracked files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jimexist commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
Jimexist commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1106030048
had to rebase to pickup fix from #2585
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jimexist commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
Jimexist commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1113880741
> After the rebase (and reformat) this seems fixed now. @ctubbsii you can verify by `git checkout master -- lib/java/src/main` and then run again `gradle spotlessApply` to see that all code changes are indeed auto-format.
well actually that was a lie - for only one of the file i indeed need to remove the wildcard import as to remove ambiguity of `String` generated by thrift and `java.lang.String`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jimexist commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
Jimexist commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1118078711
> As I understand it, `gradle build` will, among other things, run `spotlessApply`, then `spotlessCheck`.
thanks for the detailed explanation - i believe we arrived at an mutual understanding. the fact is, `gradle build` runs `gradle check` which in turns _only_ runs `spotlessCheck`, _not_ `spotlessApply`, so there's no worry that a sweep of `gradle build` will format _and then_ check. If the code is checked in unformatted, the build **will** fail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ctubbsii commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1116083880
> The specific command is gradle spotlessCheck for checking which compliments gradle spotlessApply which applies the format, the former is by default included into gradle build and will do nothing is format is correct otherwise will fail gradle build
What I'm suggesting is that the check should verify that the code that is checked into the repo is formatted, not the code that is the result of the build is formatted. That implies that spotlessCheck should run as an independent check, without running spotlessApply.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jimexist commented on pull request #2581: THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format
Posted by GitBox <gi...@apache.org>.
Jimexist commented on PR #2581:
URL: https://github.com/apache/thrift/pull/2581#issuecomment-1116071334
The specific command is gradle spotlessCheck for checking which compliments gradle spotlessApply which applies the format, the former is by default included into gradle build and will do nothing is format is correct otherwise will fail gradle build
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org