HBASE-26899 Run spotless:apply #4312
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I downloaded the newest eclipse, imported our formatter file, did some modifications, and then exported it and replaced our current formatter file. What I changed are:
Let's see the style check result. |
|
🎊 +1 overall
This message was automatically generated. |
|
Let me check the while space issue. In my local test, eclipse will generate whitespace ending lines when formatting the 'pre' tag, but if we already have no whitespace endling blank lines before and after the 'pre' tag, it will not adding new whitespaces to the line. And let me also check the indent error of checkstyle. |
|
More changes:
Which is really annoying. So I change the config of 'Parentheses positions' for most elements to 'separate lines if wrapped'. In this way the indent will be correct. The code is like this For me I think it is kinda acceptable. Let's see the result. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Seems the mvnsite result is not very stable. On master branch it could also fail. The spotbugs problem is not introduced by the reformat. It says we miss to check the return value of await but looking at the code, I think the code is correct, we do not need to check the return value. Let me think if there is a better way instead of just suppress the warning. And there are still some indentation problem. Let me take a look on them. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
There is a problem that the spotless plugin does not support exclude, for example, the thrift generated files. Let me see how to deal with this. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
OK, good, seems worked. Why we still touch the generated files and package-info.java is because in the formats section I do not exclude them from removing trailing whitespaces. @ndimiduk @apurtell PTAL whether this is enough to do a full reformat. And I've already filed HBASE-26915 for addressing some nasty style issues which should be done before landing this PR. Thanks. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Updated the PR. PTAL again? @bbeaudreault @ndimiduk Thanks~ |
This was referenced Apr 13, 2022
|
Spot checked some files, mostly lgtm but I am not a fan of this particular change: -> |
|
💔 -1 overall
This message was automatically generated. |
Agreed @apurtell , this is one of our points of discussion earlier in the thread (try to read between the buildbot spam). |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
I do not get the point here... In the past we DO wrap the method calls like this And in this issue you mentioned you do not like this style https://issues.apache.org/jira/browse/HBASE-26617 So you only want to the chained method calls to be one call per line, but for the arguments, you still want them to keep the old style? Just let me know. It is just a formatter config change... Thanks. |
|
Ping @ndimiduk |
|
@Apache9 These are my preferences. I don't know how expressive the Eclipse Formatter configuration language is... For method invocation arguments, I usually prefer as little whitespace as possible, wrapping only where necessary. Something like, However, when invoking other methods as method (or commonly, constructor) parameters, I prefer a form with more whitespace, For method declarations, I follow two styles. For simple functions where declaring a parameter type and name doesn't use very many characters, I will do a "basic" version. But when the type parameter declarations take a lot of space, I prefer a style with more whitespace. So, For method call chains with only a two method calls, I'm fine with keeping that to a single line. If there are many steps or steps that themselves wrap call chains, my preferences is one method call per line, with appropriate indentation. for example, |
|
The eclipse formatter can not express the above things exactly, let me show you some examples. This is This is This is This is This is For me I think either we choose the first one, or the last one. The last one will have more lines but I think it is more clear if we have lots of arguments. |
I'm not really satisfied by either -- the first is good for most scenarios but the last is better when there's lots of longer arguments. If I must choose one, I choose the first as it's likely the correct choice most of the time. At this point, I wonder if it's worth it for us as a community to maintain our own custom style guidelines. Perhaps we should adopt the Google Java Style, use the Google Java Format Tool, and move on. The former is well known and familiar to most devs by this point, and also is mostly identical to what we already do as a community. The latter is supported by all the major built tools via spotless plugins, as well as by both major IDEs. Thanks again, @Apache9, for spending time on this. |
Using google-java-format was my first proposal :) See this thread: |
|
OK, I think the winner is 'wrap where necessary'. Let me change the formatter file and update the PR here. And let me post an email in the dev list about this change, to receive more feedbacks before finally merging the PR. This is a very huge change on our code base, and will have long term effect, so I think we should be patient. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Seems no big concerns from the community on the mailing list. Then let's go with wrap where necessary? Since the PR will have conflicts soon after there are new commits coming in as it touch almost all the files, I plan to just push it through command line after executing 'mvn spotless:apply' locally. Plan to do it on Sunday. Shout if you guys have any concerns. Thanks @apurtell @ndimiduk @bbeaudreault for helping here! |
|
Thank you @Apache9 for landing this effort! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.