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

Changes to keep Checkstyle happy after reformatting #76464

Merged
merged 7 commits into from
Aug 18, 2021

Conversation

pugnascotia
Copy link
Contributor

@pugnascotia pugnascotia commented Aug 12, 2021

Make a number of tweaks to the source so that in the future, after a complete reformat with Spotless, Checkstyle is still happy with the code.

Also:

  • Prevent formatting of ANTLR generated source
  • Rework how test payloads are generated in MachineLearningGetResultsIT since it was unreadable.
  • Add helpers to XContentBuilder that will format better

@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Aug 12, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -169,6 +169,12 @@ dependencies {
String grammarPath = 'src/main/antlr'
String outputPath = 'src/main/java/org/elasticsearch/painless/antlr'

spotless {
java {
targetExclude "${outputPath}/*.java"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ug, we really should be putting generated source elsewhere instead of mixing it with the main sourceset. Let's add a TODO here.

@mark-vieira
Copy link
Contributor

I just remembered, should we follow up and add this to .git-blame-ignore-revs?

@pugnascotia
Copy link
Contributor Author

I just remembered, should we follow up and add this to .git-blame-ignore-revs?

I see why we might do that, but I've edited config files as well, and I'd still want those changes to show up in git blame, so I'm inclined not to add it. Plus I did a bit of refactoring in MachineLearningGetResultsIT.java 😁

@pugnascotia pugnascotia added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Aug 18, 2021
@pugnascotia
Copy link
Contributor Author

@elasticmachine update branch

@elasticsearchmachine elasticsearchmachine merged commit d01efa4 into elastic:master Aug 18, 2021
pugnascotia added a commit to pugnascotia/elasticsearch that referenced this pull request Aug 18, 2021
* Reformatting to keep Checkstyle after formatting
* Configure spotless everywhere, and disable the tasks if necessary
* Add XContentBuilder helpers, fix test
elasticsearchmachine pushed a commit that referenced this pull request Aug 18, 2021
* Reformatting to keep Checkstyle after formatting
* Configure spotless everywhere, and disable the tasks if necessary
* Add XContentBuilder helpers, fix test
@pugnascotia pugnascotia deleted the formatting-fixes branch September 14, 2021 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Delivery/Tooling Developer tooliing and automation >refactoring Team:Delivery Meta label for Delivery team v7.15.0 v8.0.0-alpha2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants