You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by GitBox <gi...@apache.org> on 2020/01/11 04:03:16 UTC
[GitHub] [incubator-hudi] leesf opened a new pull request #1208: [HUDI-304]
Bring back spotless plugin
leesf opened a new pull request #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208
## What is the purpose of the pull request
Bring back the spotless plugin, first introduced in #945 . Would run `mvn spotless:apply` to fix code format error automatically.
## Brief change log
- *Introduce eclipse-java-google-style.xml*
- *Comment LineLength module in checkstyle.xml*
## Verify this pull request
This pull request is a trivial rework / code cleanup without any test coverage.
## Committer checklist
- [ ] Has a corresponding JIRA in PR title & commit
- [ ] Commit message is descriptive of the change
- [ ] CI is green
- [ ] Necessary doc changes done or have another open PR
- [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] leesf commented on a change in pull request #1208:
[HUDI-304] Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
leesf commented on a change in pull request #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#discussion_r365633211
##########
File path: style/checkstyle.xml
##########
@@ -61,10 +61,11 @@
<property name="allowByTailComment" value="true"/>
<property name="allowNonPrintableEscapes" value="true"/>
</module>
+ <!-- when use mvn spotless:apply to format code, the length would be more than 120, even 200.
<module name="LineLength">
- <property name="max" value="200"/>
+ <property name="max" value="120"/>
<property name="ignorePattern" value="^ *\* *[^ ]+$"/>
- </module>
+ </module>-->
Review comment:
In fact, change to 120 or keep as is(200) doesn't matter since the rule is commented, it makes no sense. Here are my thoughts. When using `mvn spotless:apply` to auto fix style error, a line length would be 124 (https://github.com/apache/incubator-hudi/blob/master/hudi-common/src/test/java/org/apache/hudi/common/util/collection/TestExternalSpillableMap.java#L72), and would be 132(https://github.com/apache/incubator-hudi/blob/master/hudi-client/src/main/java/org/apache/hudi/func/CopyOnWriteLazyInsertIterable.java#L95), So seems like after formatting, there is a possibility of line length more than 200 (maybe rarely), So I comment it. BTW, in most case, 200 is enough currently.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] bvaradar commented on issue #1208: [HUDI-304]
Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
bvaradar commented on issue #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#issuecomment-573503869
@leesf:
>>> Currently, when using "Reformat Code" and "Optimize Imports" in IntelliJ, the checkstyle would not pass, e.g. after reformatting the Indentation is 4 instead of 2, so it will fail. Seems like it has already happened before?
Few months back, IIRC, apart from one caveat related indentation for deep nesting fields (You can look at checkstyle-suppressions.xml, everything else used to be consistent between IntelliJ (Reformat/Optimize Imports) and checkstyle. I am pretty sure it got changed in the last few months. You can try Hudi 0.4.6/0.4.7 release branch.
One of the biggest issue has been if we do Optimize Import, compilation fails due to checkstyle settings. Can this be made consistent ?
Also, From the hudi website, https://hudi.incubator.apache.org/newsite-content/contributing#ide-setup, we mention :
"[Recommended] We have embraced the code style largely based on google format. Please setup your IDE with style files from here. These instructions have been tested on IntelliJ."
Is this still true given the changes we have done in checkstyle settings ? If checkstyle settings is not consistent with google format, then we need to fix the documentation and provide a intellij settings xml file.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] bvaradar commented on issue #1208: [HUDI-304]
Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
bvaradar commented on issue #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#issuecomment-574314755
"""The changes in this PR has nothing to do with 120 character limit. And I randomly checked some modified files, they are modified after first introduced spotless plugin at 2019/10/10. The master won't broken as this PR is rebased to master branch.""
@leesf : Doesn't this imply spotless and checkstyle constraints are different ? It could be that eclipse style specification is more stronger than checkstyle ? I just wanted to make sure that we are not running in circles here :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] leesf commented on a change in pull request #1208:
[HUDI-304] Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
leesf commented on a change in pull request #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#discussion_r365497823
##########
File path: style/eclipse-java-google-style.xml
##########
@@ -0,0 +1,351 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+ http://www.apache.org/licenses/LICENSE-2.0
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
Review comment:
Seems that Apache License is OK, also checked the Apache Avro https://github.com/apache/avro/blob/8026c8ffe4ef67ab419dba73910636bf2c1a691c/lang/java/eclipse-java-formatter.xml cc @bvaradar
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] bvaradar commented on a change in pull request
#1208: [HUDI-304] Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
bvaradar commented on a change in pull request #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#discussion_r366507169
##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/HoodieCLI.java
##########
@@ -64,8 +64,8 @@ private static void setBasePath(String basePath) {
}
private static void setLayoutVersion(Integer layoutVersion) {
- HoodieCLI.layoutVersion = new TimelineLayoutVersion(
- (layoutVersion == null) ? TimelineLayoutVersion.CURR_VERSION : layoutVersion);
+ HoodieCLI.layoutVersion =
Review comment:
For example, I am quoting this line. Do you know what rule in eclipse created this change. As checkstyle never enforced it, can we remove this rule ? Please take a look at other changes to see if there is any other pattern like 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] leesf commented on issue #1208: [HUDI-304] Bring
back spotless plugin
Posted by GitBox <gi...@apache.org>.
leesf commented on issue #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#issuecomment-573493705
> Thanks @leesf . The current IDE setup steps (https://hudi.incubator.apache.org/newsite-content/contributing) is inconsistent with the style settings present in the repository.
>
> We need to ensure the convention we enforce needs to be auto-settable in IDE (using IDE code-formatting options). For example : in IntelliJ, if we "Reformat Code" and "Optimize Imports", it should bring the code to a state where checkstyle needs to pass. This is how it used to be with minor caveats. Can we ensure we reach that state.
>
> What we need is :
>
> 1. As spotless relies on eclipse style config, make changes to eclipse style config to make it consistent with current checkstyle setting so that we don't end up with massive change
> 2. Also provide corresponding intellij settings (there are many intellij users here :) ) which is again consistent with eclipse style settings enforced by spotless
> 3. Update https://hudi.incubator.apache.org/newsite-content/contributing to reflect how to set code style. For e.g: We are no longer using vanilla google style (which has 120 char limit for line boundary whereas we need 200 char limit) and so we cannot ask developers to use google code style directly.
>
> Let me your thoughts on this ?
Thanks for your comments.
Sorry I would not get the point that The current IDE setup steps (https://hudi.incubator.apache.org/newsite-content/contributing) is inconsistent with the style settings present in the repository?
1. Currently, when using "Reformat Code" and "Optimize Imports" in IntelliJ, the checkstyle would not pass, e.g. after reformatting the Indentation is 4 instead of 2, so it will fail. Seems like it has already happened before?
2. The rules introduced in eclipse-java-google-style.xml are compatible with rules in checkstyle.xml.
3. In my local dev, I use checkstyle plugin and the checkstyle.xml in (https://github.com/apache/incubator-hudi/tree/master/style), it plays well with spotless plugin(eclipse-java-google-style.xml) and checkstyle(checkstyle.xml).
Please correct me if I am wrong.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] leesf commented on issue #1208: [HUDI-304] Bring
back spotless plugin
Posted by GitBox <gi...@apache.org>.
leesf commented on issue #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#issuecomment-573490929
> Could you comment on the state of code formatting, if the PR is merged? Specifically
>
> * are all checkstyle error "fully" auto fixable using spotless or there are exceptions?
> * are developers supposed to use the eclipse-java-google-style for formatting code or use checkstyle rules?
> * what about intellij?
> * can we update the contributing guide accordingly
* Not all error would be fixed such as import order, but as we have ignore the import order rule, so it plays well with other rules.
* Users would still use checkstyle rules of checkstyle.xml, and the introduced eclipse-java-google-style.xml is compatible with the rules of check.xml.
* idea set up would still refer to (https://hudi.incubator.apache.org/newsite-content/contributing).
* Yes, we would document how to fix code format error by using `mvn spotless:apply`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] leesf edited a comment on issue #1208: [HUDI-304]
Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
leesf edited a comment on issue #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#issuecomment-574102314
> > ocument that developers could use checkstyle.xml file in style folder in checkstyle plugin and things will go well
>
> I was able to use checkstyle to format in IntelliJ. This is fine.. but we should clearly document this. maybe file a JIRA?
>
> On import order, we can take a second stab may be down the line? again filing a JIRA would be great for tracking..
>
> On this PR, my concern was we are reformatting again due to the 120 character limit? I was trying to see if we can avoid it. @leesf could you explain why 100+ files are being touched in this PR? If these were all checkstyle failures, then master would be broken right? I am just trying to understand what code really changed here, given we are close to a release..
Created https://issues.apache.org/jira/projects/HUDI/issues/HUDI-533 and https://issues.apache.org/jira/projects/HUDI/issues/HUDI-534 to track these.
The changes in this PR has nothing to do with 120 character limit. And I randomly checked some modified files, they are modified after first introduced spotless plugin at 2019/10/10. The master won't broken as this PR is rebased to master branch.
PS: And my another question is that if we need to modify `checkstyle-suppressions.xml` to make Reformat in IntelliJ and checkstyle.xml pass? cc @bvaradar @vinothchandar
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] leesf commented on issue #1208: [HUDI-304] Bring
back spotless plugin
Posted by GitBox <gi...@apache.org>.
leesf commented on issue #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#issuecomment-573499676
Thanks for your comments @vinothchandar @bvaradar and remindar @lamber-ken , updated the PR to keep the lineLength 200.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] leesf commented on issue #1208: [HUDI-304] Bring
back spotless plugin
Posted by GitBox <gi...@apache.org>.
leesf commented on issue #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#issuecomment-573489191
> I think most of the changes here are due to the 200 -> 120 line length change? In the interest of not reformatting code back and forth, I think we should just keep `200` . 200 is also a better practical value IMO. most modern displays can easily support 200 characters without scroll..
Sure, 200 is enough as explained above.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] leesf commented on a change in pull request #1208:
[HUDI-304] Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
leesf commented on a change in pull request #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#discussion_r365497747
##########
File path: style/checkstyle.xml
##########
@@ -61,10 +61,11 @@
<property name="allowByTailComment" value="true"/>
<property name="allowNonPrintableEscapes" value="true"/>
</module>
+ <!-- when use mvn spotless:apply to format code, the length would be more than 120, even 200.
<module name="LineLength">
- <property name="max" value="200"/>
+ <property name="max" value="120"/>
<property name="ignorePattern" value="^ *\* *[^ ]+$"/>
- </module>
+ </module>-->
Review comment:
Comment it intentionally.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] leesf edited a comment on issue #1208: [HUDI-304]
Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
leesf edited a comment on issue #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#issuecomment-574102314
> > ocument that developers could use checkstyle.xml file in style folder in checkstyle plugin and things will go well
>
> I was able to use checkstyle to format in IntelliJ. This is fine.. but we should clearly document this. maybe file a JIRA?
>
> On import order, we can take a second stab may be down the line? again filing a JIRA would be great for tracking..
>
> On this PR, my concern was we are reformatting again due to the 120 character limit? I was trying to see if we can avoid it. @leesf could you explain why 100+ files are being touched in this PR? If these were all checkstyle failures, then master would be broken right? I am just trying to understand what code really changed here, given we are close to a release..
Created https://issues.apache.org/jira/projects/HUDI/issues/HUDI-533 and https://issues.apache.org/jira/projects/HUDI/issues/HUDI-534 to track these.
The changes in this PR has nothing to do with 120 character limit. And I randomly checked some modified files, they are modified after first introduced spotless plugin at 2019/10/10. The master won't broken as this PR is rebased to master branch.
PS: And my another question is that if we need to modify `checkstyle-suppressions.xml` to make Reformat in IntelliJ and checkstyle.xml pass? but the spotless plugin would fail because of the identation is 4 after Reformat and the checkstyle.xml would also pass because of `<suppress checks="indentation" files="." lines="1-9999"/>` in checkstyle-suppressions.xml, but the spotless plugin would fail and I would not find a way to make three all pass (above jira tickets to tracking this to document it clearly). But after running `mvn spotless:apply`, we could make build pass. cc @bvaradar @vinothchandar
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hudi] vinothchandar closed pull request #1208: [WIP] [HUDI-304] Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
vinothchandar closed pull request #1208:
URL: https://github.com/apache/hudi/pull/1208
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hudi] leesf commented on issue #1208: [HUDI-304] Bring
back spotless plugin
Posted by GitBox <gi...@apache.org>.
leesf commented on issue #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#issuecomment-574102314
> > ocument that developers could use checkstyle.xml file in style folder in checkstyle plugin and things will go well
>
> I was able to use checkstyle to format in IntelliJ. This is fine.. but we should clearly document this. maybe file a JIRA?
>
> On import order, we can take a second stab may be down the line? again filing a JIRA would be great for tracking..
>
> On this PR, my concern was we are reformatting again due to the 120 character limit? I was trying to see if we can avoid it. @leesf could you explain why 100+ files are being touched in this PR? If these were all checkstyle failures, then master would be broken right? I am just trying to understand what code really changed here, given we are close to a release..
Created https://issues.apache.org/jira/projects/HUDI/issues/HUDI-533 and https://issues.apache.org/jira/projects/HUDI/issues/HUDI-534 to track these.
The changes in this PR has nothing to do with 120 character limit. And I randomly checked some modified files, they are modified after first introduced spotless plugin at 2019/10/10. The master won't broken as this PR is rebased to master branch.
PS: And my another question is that if we need to modify `checkstyle-suppressions.xml` to make Reformat in IntelliJ, checkstyle.xml and eclipse-java-google-style.xml all pass? cc @bvaradar @vinothchandar
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] leesf edited a comment on issue #1208: [HUDI-304]
Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
leesf edited a comment on issue #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#issuecomment-573490929
> Could you comment on the state of code formatting, if the PR is merged? Specifically
>
> * are all checkstyle error "fully" auto fixable using spotless or there are exceptions?
> * are developers supposed to use the eclipse-java-google-style for formatting code or use checkstyle rules?
> * what about intellij?
> * can we update the contributing guide accordingly
* Not all error would be fixed such as import order, but as we have ignore the import order rule, so it plays well with other rules.
* Users would still use checkstyle rules of checkstyle.xml, and the introduced eclipse-java-google-style.xml is compatible with the rules of check.xml.
* idea set up would still refer to (https://hudi.incubator.apache.org/newsite-content/contributing).
* Yes, we would document how to fix code format error by using `mvn spotless:apply`.
WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] lamber-ken commented on issue #1208: [HUDI-304]
Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
lamber-ken commented on issue #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#issuecomment-573389668
Here are some useful tools which can be placed at contributing guide.
**1, Google styleguide**
https://google.github.io/styleguide/javaguide.html
**2, Idea tool**
https://plugins.jetbrains.com/plugin/8527-google-java-format
**3, Java tool**
https://github.com/google/google-java-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] vinothchandar commented on a change in pull
request #1208: [HUDI-304] Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#discussion_r365560615
##########
File path: style/checkstyle.xml
##########
@@ -61,10 +61,11 @@
<property name="allowByTailComment" value="true"/>
<property name="allowNonPrintableEscapes" value="true"/>
</module>
+ <!-- when use mvn spotless:apply to format code, the length would be more than 120, even 200.
<module name="LineLength">
- <property name="max" value="200"/>
+ <property name="max" value="120"/>
<property name="ignorePattern" value="^ *\* *[^ ]+$"/>
- </module>
+ </module>-->
Review comment:
why the change to 120?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hudi] vinothchandar commented on pull request #1208: [WIP] [HUDI-304] Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on pull request #1208:
URL: https://github.com/apache/hudi/pull/1208#issuecomment-774540743
Closing this in favor of the work @xushiyan is now pursuing
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hudi] vinothchandar commented on issue #1208: [HUDI-304]
Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on issue #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#issuecomment-574024869
>ocument that developers could use checkstyle.xml file in style folder in checkstyle plugin and things will go well
I was able to use checkstyle to format in IntelliJ. This is fine.. but we should clearly document this. maybe file a JIRA?
On import order, we can take a second stab may be down the line? again filing a JIRA would be great for tracking..
On this PR, my concern was we are reformatting again due to the 120 character limit? I was trying to see if we can avoid it. @leesf could you explain why 100+ files are being touched in this PR? If these were all checkstyle failures, then master would be broken right? I am just trying to understand what code really changed here, given we are close to a release..
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] leesf commented on issue #1208: [HUDI-304] Bring
back spotless plugin
Posted by GitBox <gi...@apache.org>.
leesf commented on issue #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#issuecomment-573539453
> @leesf:
>
> > > > Currently, when using "Reformat Code" and "Optimize Imports" in IntelliJ, the checkstyle would not pass, e.g. after reformatting the Indentation is 4 instead of 2, so it will fail. Seems like it has already happened before?
>
> Few months back, IIRC, apart from one caveat related indentation for deep nesting fields (You can look at checkstyle-suppressions.xml, everything else used to be consistent between IntelliJ (Reformat/Optimize Imports) and checkstyle. I am pretty sure it got changed in the last few months. You can try Hudi 0.4.6/0.4.7 release branch.
>
> One of the biggest issue has been if we do Optimize Import, compilation fails due to checkstyle settings. Can this be made consistent ?
>
> Also, From the hudi website, https://hudi.incubator.apache.org/newsite-content/contributing#ide-setup, we mention :
>
> "[Recommended] We have embraced the code style largely based on google format. Please setup your IDE with style files from here. These instructions have been tested on IntelliJ."
>
> Is this still true given the changes we have done in checkstyle settings ? If checkstyle settings is not consistent with google format, then we need to fix the documentation and provide a intellij settings xml file.
Hi @bvaradar After checking the hoodie-0.4.6 and hoodie-0.4.7 branch and runnig Reformat in IntelliJ, it still fails runing `mvn clean install -DskipTests` with indentation error. Could you please help me verify again?
In the documentation, I think we need to document that developers could use checkstyle.xml file in style folder in checkstyle plugin and things will go well since the eclipse-java-google-style.xml is compatible with checkstyle.xml.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] leesf edited a comment on issue #1208: [HUDI-304]
Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
leesf edited a comment on issue #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#issuecomment-573539453
> @leesf:
>
> > > > Currently, when using "Reformat Code" and "Optimize Imports" in IntelliJ, the checkstyle would not pass, e.g. after reformatting the Indentation is 4 instead of 2, so it will fail. Seems like it has already happened before?
>
> Few months back, IIRC, apart from one caveat related indentation for deep nesting fields (You can look at checkstyle-suppressions.xml, everything else used to be consistent between IntelliJ (Reformat/Optimize Imports) and checkstyle. I am pretty sure it got changed in the last few months. You can try Hudi 0.4.6/0.4.7 release branch.
>
> One of the biggest issue has been if we do Optimize Import, compilation fails due to checkstyle settings. Can this be made consistent ?
>
> Also, From the hudi website, https://hudi.incubator.apache.org/newsite-content/contributing#ide-setup, we mention :
>
> "[Recommended] We have embraced the code style largely based on google format. Please setup your IDE with style files from here. These instructions have been tested on IntelliJ."
>
> Is this still true given the changes we have done in checkstyle settings ? If checkstyle settings is not consistent with google format, then we need to fix the documentation and provide a intellij settings xml file.
Hi @bvaradar After checking the hoodie-0.4.6 and hoodie-0.4.7 branch and runnig Reformat in IntelliJ, it still fails runing `mvn clean install -DskipTests` with indentation error. Could you please help me verify again?
After modifying the checkstyle-suppressions.xml in hoodie-0.4.7 (https://github.com/apache/incubator-hudi/blob/hoodie-0.4.7/style/checkstyle-suppressions.xml) to the following (mainly change the `HoodieLogFileCommand.java` to `.` in indentation checks)
```xml
<suppressions>
<!-- Impossible to resolve checkstyle indentation violation because of checkstyle bug -->
<suppress checks="indentation" files="." lines="1-9999"/>
<!-- Member Names expected to start with "_" -->
<suppress checks="naming" files="TestRecord.java" lines="1-9999"/>
</suppressions>
```
Reformat in IntelliJ and checkstyle.xml would play well together. Also I verify it in lastest master branch, it plays well too. So just modify the suppressions.xml file to get all make sense?
As to the documentation, I think we need to document that developers could use checkstyle.xml file in style folder in checkstyle plugin and things will go well since the eclipse-java-google-style.xml is compatible with checkstyle.xml.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] leesf edited a comment on issue #1208: [HUDI-304]
Bring back spotless plugin
Posted by GitBox <gi...@apache.org>.
leesf edited a comment on issue #1208: [HUDI-304] Bring back spotless plugin
URL: https://github.com/apache/incubator-hudi/pull/1208#issuecomment-573489191
> I think most of the changes here are due to the 200 -> 120 line length change? In the interest of not reformatting code back and forth, I think we should just keep `200` . 200 is also a better practical value IMO. most modern displays can easily support 200 characters without scroll..
No the changes has nothing to do with changing lineLength to 120.
Sure, 200 is enough as explained above.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services