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

CodeStyle formatting to conform to basic Checkstyle rules. #360

Merged
merged 1 commit into from Mar 30, 2018

Conversation

bvaradar
Copy link
Contributor

This diff contains formatting changes to adhere to basic checkstyle rules (conforming to google-format).

I am adding the checkstyle and corresponding eclipse/IntelliJ code-styles under "style/" folder so that all of us can use the same ide code-styles. I have made few changes to google-format checkstyle:

  1. Increase line length from 100 to 120
  2. Comment out JavaDoc related checkstyles as this needs more manual work.

I have added 2 files to checkstyle suppressions list (style/checkstyle-suppressions.xml). One of them was because of checkstyle bug. Please keep in mind that checkstyle is somewhat of a super-set compared to code-styles when it comes to enforcing rules.

With the maven settings, now "mvn compile" will fail if there are any checkstyle violations.

Major theme in the below diff as suggested by checkstyle:

  1. Line limit to 120
  2. Lambda Expression Indentation (Please note that IntelliJ does not have good support for this. In some cases, we need to manually indent).
  3. Moving top-level non-public classes to separate files
  4. Using default (in switch) to cover bases.
  5. Modifier Ordering (final static vs static final)
  6. Local Variable Naming

@n3nash
Copy link
Contributor

n3nash commented Mar 21, 2018

@jianxu FYI since this might cause merging/syncing with internal repo.

@jianxu
Copy link
Contributor

jianxu commented Mar 22, 2018

thanks @n3nash! @bvaradar is there way to apply the coding style change without manually indent for some cases?

@bvaradar
Copy link
Contributor Author

@jianxu : Other than multi-line lambda expressions and very deep-nested generics based expressions, the indentations should work automatically if you use the style/intellij-java-google-style.xml (in this PR) as code-style (for intellij) and do Reformat-Code.

For IntelliJ Ultimate 2017.3, the way to setupis Preferences -> Editor -> Code Style -> Java -> Import Schema (under Settings icon) -> IntelliJ IDEA Code Style XML

@jianxu
Copy link
Contributor

jianxu commented Mar 22, 2018

@bvaradar Thanks for the info, maybe also update https://github.com/uber/hudi/blob/master/docs/dev_setup.md ?

@bvaradar
Copy link
Contributor Author

Thanks @jianxu. I have updated the doc.

@n3nash
Copy link
Contributor

n3nash commented Mar 22, 2018

@bvaradar it LGTM. Can you hold off on merging this till I merge the 1 large PR I have. I would like to avoid dealing with conflicts if possible, thanks

<AND>
<NAME>.*:id</NAME>
<XML_ATTRIBUTE />
<XML_NAMESPACE>http://schemas.android.com/apk/res/android</XML_NAMESPACE>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be there by default ?

@bvaradar

Copy link
Contributor Author

@bvaradar bvaradar Mar 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@vinothchandar vinothchandar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirming, you have brought checkstyle formatting and the intellij formatting in sync, correct?

As long as one sets up IDE with the checked in style, it should all be fine?

@bvaradar
Copy link
Contributor Author

bvaradar commented Mar 23, 2018

@vinothchandar : Both specifications are aligned to the extent they are not contradicting each other. But there are some exceptions.

  1. there is an issue with checkstyle causing a generic indentation rule to fail for some nested lambda expressions (have added suppression for one case).
  2. There are below caveats as I could not express some checkstyle rules in code-style (but felt should be good to have). They are:
    (a) Moving top-level non-public classes to separate files
    (b) Using default in switch
    (c) Local Variables Naming

@vinothchandar
Copy link
Member

sg.. can you rebase this once #350 is merged.. I think this is what @n3nash is hinting at.

@vinothchandar
Copy link
Member

@n3nash can @bvaradar merge this now? @bvaradar please rebase this based off latest master since a few commits have a gone in..

@bvaradar bvaradar force-pushed the format_rules branch 2 times, most recently from 3feb3c6 to a1ae125 Compare March 29, 2018 23:40
@vinothchandar
Copy link
Member

@jianxu any concerns before I merge this? @n3nash ?

@n3nash
Copy link
Contributor

n3nash commented Mar 30, 2018

@vinothchandar No objections from my end. @bvaradar BTW, does this take care of unused imports too ?

The code-style rules follow google style with some changes:

1. Increase line length from 100 to 120
2. Disable JavaDoc related checkstyles as this needs more manual work.

Both source and test code are checked for code-style
@bvaradar
Copy link
Contributor Author

@n3nash : Thanks. Added both Unused and Redundant Imports in checkstyle and corresponding code-style. If there are any other rules missing, we can add them in future PRs.

@jianxu
Copy link
Contributor

jianxu commented Mar 30, 2018

@vinothchandar @bvaradar sounds good to me. Let's also make sure all pending PRs are rebased after this change is merged.

cc @kaushikd49 @ovj please try to rebase your changes on top of this PR and also setup your IDEs correspondingly.

@vinothchandar vinothchandar merged commit 788e4f2 into master Mar 30, 2018
@vinothchandar vinothchandar deleted the format_rules branch April 2, 2018 16:00
vinishjail97 pushed a commit to vinishjail97/hudi that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants