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

PHOENIX-7441 Integrate the Spotless plugin and update the code template #2023

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

NihalJain
Copy link
Contributor

@NihalJain NihalJain commented Nov 6, 2024

  • Update dev/PhoenixCodeTemplate.xml to use latest format as in hbase.
  • Copied license-header file from hbase, the one which phoenix was using was a little different
  • Fix file having misplaced package block as we do not want any manual code change in the commit where we run spotless
  • Disable upToDateChecking which is true by default since spotless 2.35.0

- Update dev/PhoenixCodeTemplate.xml to use latest format as in hbase.
- Copied license-header file from hbase, the one which phoenix was using was a little different
- Fix file having misplaced package block as we do not want any manual code change in the commit where we run spotless
@NihalJain
Copy link
Contributor Author

NihalJain commented Nov 6, 2024

I have updated to latest spotless as suggested by Lars.
Question to reviewers:

  • Revisit the import order rules?
  • Also IMO we should rename file PhoenixCodeTemplate.xml to phoenix_eclipse_formatter.xml?

CC: @stoty @lfrancke

</licenseHeader>
</format>
</formats>
<!-- Spotless considers processing same file twice: a bug!
Copy link
Contributor Author

@NihalJain NihalJain Nov 6, 2024

Choose a reason for hiding this comment

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

This change is to handle spotless 2.35.0+ as java formatting breaks for us w/o below change on newer spotless versions. Refer diffplug/spotless#1767

Copy link
Contributor Author

@NihalJain NihalJain Nov 6, 2024

Choose a reason for hiding this comment

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

Upon downgrading to 2.30.0 due to java 8, this change should no longer needed but good to have it to ensure we don't break again in future when we move to 2.35.0+

specific language governing permissions and limitations
under the License.

/**
Copy link
Contributor Author

@NihalJain NihalJain Nov 6, 2024

Choose a reason for hiding this comment

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

pom.xml Outdated
@@ -168,6 +168,7 @@
<maven-checkstyle-plugin.version>3.3.0</maven-checkstyle-plugin.version>
<maven-shade-plugin.version>3.6.0</maven-shade-plugin.version>
<mvel2.version>2.5.2.Final</mvel2.version>
<spotless.version>2.43.0</spotless.version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might break for JDK 8. 😞

[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.43.0:apply (default-cli) on project phoenix: Execution default-cli of goal com.diffplug.spotless:spotless-maven-plugin:2.43.0:apply failed: Unable to load the mojo 'apply' in the plugin 'com.diffplug.spotless:spotless-maven-plugin:2.43.0' due to an API incompatibility: org.codehaus.plexus.component.repository.exception.ComponentLookupException: com/diffplug/spotless/maven/SpotlessApplyMojo has been compiled by a more recent version of the Java Runtime (class file version 55.0), this version of the Java Runtime only recognizes class file versions up to 52.0

Copy link
Contributor Author

@NihalJain NihalJain Nov 6, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Downgraded to 2.30.0

@stoty
Copy link
Contributor

stoty commented Nov 7, 2024

I would either discuss the import order changes in the existing dev thread, or open a new one for them.
We should have consensus on that before the big bang reformat commit. (Even if it's just no objection/replies)
I personally support copying everything from HBase.

I'm fine with renaming the xml, but if we do that, we also need to update the website, which refers to it.

@stoty
Copy link
Contributor

stoty commented Nov 7, 2024

PTAL @virajjasani @gjacoby126

@lfrancke
Copy link
Member

lfrancke commented Nov 7, 2024

@stoty Got to this before me and I agree with everything he said: Just copying from HBase seems like a good idea to me as well.

This change looks good to me in principle (and thanks for catching that JDK incompatiblity).
Thanks for the work!

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.

3 participants