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

Removed CheckStyle code formatting plugin #1370

Merged

Conversation

owaiskazi19
Copy link
Member

@owaiskazi19 owaiskazi19 commented Oct 14, 2021

Signed-off-by: Owais Kazi owaiskazi19@gmail.com

Description

Currently the project has two formatting tools Spotless and CheckStyle. Spotless itself can format the code and check the standard java code using spotlessApply and spotlessJavaCheck respectively. Both the plugins have different configuration for formatting which restrict us to run Spotless on all the subprojects. Removing CheckStyle will help to run a unified formatting on the codebase.

Issues Resolved

#1362

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@owaiskazi19 owaiskazi19 changed the title Cleanup for Checkstyle Removed CheckStyle code formatting plugin Oct 14, 2021
@opensearch-ci-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed da79ad2f97df83b62bd6ca215100064b72bebeef

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success da79ad2f97df83b62bd6ca215100064b72bebeef

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success da79ad2f97df83b62bd6ca215100064b72bebeef

Copy link
Collaborator

@VachaShah VachaShah left a comment

Choose a reason for hiding this comment

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

Between the time that we remove checkstyle and add spotless to the entire repo, would the modules excluded from spotless have no formatting standards?

@owaiskazi19
Copy link
Member Author

owaiskazi19 commented Oct 14, 2021

Between the time that we remove checkstyle and add spotless to the entire repo, would the modules excluded from spotless have no formatting standards?

Good question. Few are already formatted with CheckStyle and Spotless and few are not formatted by any plugins is what I can think of. Once we apply Spotless on all the modules they'll follow https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/formatterConfig.xml.
Moreover, spotlessJavaCheck runs as a part of precommit and check. So, it'll take care of the formatting of the included modules.

@tlfeng
Copy link
Collaborator

tlfeng commented Oct 14, 2021

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success da79ad2f97df83b62bd6ca215100064b72bebeef
Log 694

Reports 694

@tlfeng tlfeng added Build Libraries & Interfaces v2.0.0 Version 2.0.0 v1.2.0 Issues related to version 1.2.0 labels Oct 15, 2021
@VachaShah
Copy link
Collaborator

VachaShah commented Oct 15, 2021

Between the time that we remove checkstyle and add spotless to the entire repo, would the modules excluded from spotless have no formatting standards?

Good question. Few are already formatted with CheckStyle and Spotless and few are not formatted by any plugins is what I can think of. Once we apply Spotless on all the modules they'll follow https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/formatterConfig.xml. Moreover, spotlessJavaCheck runs as a part of precommit and check. So, it'll take care of the formatting of the included modules.

Yeah my concern is the new changes in the modules that are excluded and won't have neither checkstyle nor spotless running on them. Otherwise the PR looks fine to me.

@owaiskazi19
Copy link
Member Author

owaiskazi19 commented Oct 15, 2021

Between the time that we remove checkstyle and add spotless to the entire repo, would the modules excluded from spotless have no formatting standards?

Good question. Few are already formatted with CheckStyle and Spotless and few are not formatted by any plugins is what I can think of. Once we apply Spotless on all the modules they'll follow https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/formatterConfig.xml. Moreover, spotlessJavaCheck runs as a part of precommit and check. So, it'll take care of the formatting of the included modules.

Yeah my concern is the modules that are excluded and won't have neither checkstyle nor spotless running on them. Otherwise the PR looks fine to me.

So those modules are already formatted with CheckStyle as far as I know until the time we apply Spotless to them.

@owaiskazi19 owaiskazi19 requested a review from tlfeng October 15, 2021 21:54
@setiah
Copy link
Contributor

setiah commented Oct 18, 2021

Between the time that we remove checkstyle and add spotless to the entire repo, would the modules excluded from spotless have no formatting standards?

Good question. Few are already formatted with CheckStyle and Spotless and few are not formatted by any plugins is what I can think of. Once we apply Spotless on all the modules they'll follow https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/formatterConfig.xml. Moreover, spotlessJavaCheck runs as a part of precommit and check. So, it'll take care of the formatting of the included modules.

Yeah my concern is the modules that are excluded and won't have neither checkstyle nor spotless running on them. Otherwise the PR looks fine to me.

So those modules are already formatted with CheckStyle as far as I know until the time we apply Spotless to them.

Sorry about repeating this but not sure if I understood correctly. Have we made sure removing checkstyle does not create a gap with no styling checks on some modules which were previously checkstyle based?

@owaiskazi19
Copy link
Member Author

As discussed with @setiah offline. We will be excluding few modules to run CheckStyle and will run Spotless on them to remove the gap of formatting. Let's hold this PR and merge this after all the 74 modules are sync with Spotless.

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success da79ad2f97df83b62bd6ca215100064b72bebeef

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed da79ad2f97df83b62bd6ca215100064b72bebeef

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success da79ad2f97df83b62bd6ca215100064b72bebeef

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 3e9db70

@owaiskazi19 owaiskazi19 requested review from setiah and removed request for adnapibar November 2, 2021 01:29
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 3e9db70

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 3e9db70
Log 949

Reports 949

@tlfeng
Copy link
Collaborator

tlfeng commented Nov 2, 2021

start gradle check

Copy link
Member

@saratvemulapalli saratvemulapalli left a comment

Choose a reason for hiding this comment

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

Changes look good to me!

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 3e9db70
Log 953

Reports 953

@saratvemulapalli saratvemulapalli merged commit a439371 into opensearch-project:main Nov 2, 2021
owaiskazi19 added a commit to owaiskazi19/OpenSearch that referenced this pull request Nov 2, 2021
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
saratvemulapalli pushed a commit that referenced this pull request Nov 2, 2021
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Libraries & Interfaces v1.2.0 Issues related to version 1.2.0 v2.0.0 Version 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants