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

PARQUET-2386: More consistent code style in parquet-mr #1209

Merged
merged 2 commits into from
Nov 30, 2023

Conversation

amousavigourabi
Copy link
Contributor

Make sure you have checked all steps below.

Jira

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    • This PR only refactors style, no logic is added or removed in any way shape or form

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does
    • Adds note in the PR template on the style checks

This PR contains two commits, the first adds the style checks and configurations, the second applies these changes to the repository.

@amousavigourabi
Copy link
Contributor Author

@wgtmac

@amousavigourabi
Copy link
Contributor Author

The .editorconfig has been expanded for IntelliJ and is mostly compliant with the Spotless configuration. IntelliJ refactoring and Spotless have some minor disagreements on continuation indents sometimes, which cannot really be resolved at the moment. As it is included in the Maven lifecycle, the Spotless configuration would of course be leading.

@wgtmac
Copy link
Member

wgtmac commented Nov 29, 2023

Thanks for the improvement!

Could you please take a look at this? @shangxinli @gszadovszky @Fokko

Copy link
Contributor

@gszadovszky gszadovszky left a comment

Choose a reason for hiding this comment

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

Thank you @amousavigourabi for working on this!
One comment about tabs for indention, otherwise LGTM.

Comment on lines +535 to +537
<indent>
<tabs>true</tabs>
<spacesPerTab>4</spacesPerTab>
</indent>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it mean we allow tabs for indention? (Personally, I don't like tabs because it might be rendered differently on different systems.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, tabs are not allowed by this configuration. This block is a way to make the linter work as it should with two space indents as used by Parquet, instead of four.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a bit hacky, but it also allows contributors to run the spotless:apply goal in case they are for whatever reason using tabs in their development environment.

@Fokko
Copy link
Contributor

Fokko commented Nov 29, 2023

This is great @amousavigourabi. I was thinking of adding linting as well, so great work here!

@amousavigourabi
Copy link
Contributor Author

@Fokko , how do we want to coordinate this pull request and your refactoring PRs? I do not feel a lot for applying the editorconfig and Spotless a dozen or so times to resolve conflicts in this PR after each of those get merged.

@Fokko
Copy link
Contributor

Fokko commented Nov 29, 2023

@amousavigourabi I can see that it is not nice having to rebase all the time. I can hold off on mine, they are probably easier to resolve than yours.

Add linter and associated editor configuration
Apply more consistent style using linter and editor configuration
@amousavigourabi
Copy link
Contributor Author

I rebased on the latest master. I'd rather get this merged somewhat quickly (maybe by the end of the week?) as to avoid blocking other merges and/or endless rebasing.

@wgtmac wgtmac merged commit 945836c into apache:master Nov 30, 2023
9 checks passed
@wgtmac
Copy link
Member

wgtmac commented Nov 30, 2023

I rebased on the latest master. I'd rather get this merged somewhat quickly (maybe by the end of the week?) as to avoid blocking other merges and/or endless rebasing.

I just merged this PR to unblock others. Thanks @amousavigourabi!

@amousavigourabi amousavigourabi deleted the more-consistent-code-style branch November 30, 2023 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants