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

DRAFT: Code formatting using spotless in Phoenix #1995

Closed
wants to merge 3 commits into from

Conversation

NihalJain
Copy link
Contributor

@NihalJain NihalJain commented Oct 6, 2024

This PR currently has 3 commits, each of which do independent tasks as follows:

  • Add spotless plugin to format code in phoenix
    • Update dev/PhoenixCodeTemplate.xml to use latest format as in hbase.
      • Question to reviewers: Also IMO we should rename this file to phoenix_eclipse_formatter.xml?
    • Fix file having misplaced package block as we do not want any manual code change in next commit where we run spotless
    • Copied licnese-header from hbase, the one which phoenix was using was a little different
    • Question to reviewers: Revisit the import order rules?
  • Run spotless:apply to format code in phoenix:
    • No manual code change in this commit
  • Add spotless phoenix pre commit check

We could create 3 JIRA sub tasks and do each commit one at a time for ease of backporting.

TODO: Raise JIRAs as needed

pom.xml Outdated Show resolved Hide resolved
dev/PhoenixCodeTemplate.xml Outdated Show resolved Hide resolved
@NihalJain
Copy link
Contributor Author

This code can be reviewed, just that not ready for merge. Need to add JIRA title and also decide whether should go as one commit or multi commit.

@stoty
Copy link
Contributor

stoty commented Oct 9, 2024

There was already a DISCUSS thread on this back in Januray 2023, but nothing happened.

I think that this is visible and high impact enough to warrant another [DISCUSS] thread.

I think we should do this, BTW.

@@ -167,6 +167,7 @@
<exec-maven-plugin.version>3.1.1</exec-maven-plugin.version>
<maven-checkstyle-plugin.version>3.3.0</maven-checkstyle-plugin.version>
<mvel2.version>2.5.2.Final</mvel2.version>
<spotless.version>2.27.2</spotless.version>
Copy link
Member

Choose a reason for hiding this comment

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

This is a pretty old version (from 2022). Is there a reason you're not using the latest? (2.43.0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes you are right. I just kept this in sync with hbase but yes let me bump into hbase codebase as well and move to the latest one over here as well.

@lfrancke
Copy link
Member

Thank you so much for picking this up. I just stumbled upon the previous PR, Jira issue and the previous mailing list thread.

I do appreciate the work a lot and can try to help out where I can.

I also appreciate that you split the PR up in multiple commits.
If it's not too much work splitting this up in at least two PRs would help tremendously with reviewing it.
One that does all the "machinery" and another that runs the reformat etc.

@NihalJain
Copy link
Contributor Author

There was already a DISCUSS thread on this back in Januray 2023, but nothing happened.
I think that this is visible and high impact enough to warrant another [DISCUSS] thread.
I think we should do this, BTW.

Sure @stoty Thank you for the reference. Let me chime into DISCUSS thread and put up what we plan to do here.

Thank you so much for picking this up. I just stumbled upon the previous PR, Jira issue and the previous mailing list thread.

Thank you @lfrancke for bringing this up again on DISCUSS, and for pointing out the JIRA for previous issue.

I do appreciate the work a lot and can try to help out where I can.

Sure will be great if you can help with reviews. Will let you know if anything else if needed.

I also appreciate that you split the PR up in multiple commits.
If it's not too much work splitting this up in at least two PRs would help tremendously with reviewing it.
One that does all the "machinery" and another that runs the reformat etc.

I plan to raise 3 PRs . This PR was just to demo the level of inconsistency we have in the codebase and seek feedback. Let me create appropriate JIRAs and raise 3 distinct PRs.

  1. Add spotless plugin to format code in phoenix
  2. Run spotless:apply to format code in phoenix
  3. Add spotless phoenix pre commit check

Also need to break the "machinery" part into tasks: task 1 and task 3 as we may want to add to pre-commit only after all issues are fixed with task2. This way we can have ease of backporting as well since task 2 generated code may differ for all branches.

@lfrancke
Copy link
Member

Excellent, in that case I just misread. Thank you! The plan makes sense to me.
I'm really looking forward to get this in.

@NihalJain
Copy link
Contributor Author

work superceded at #2023

@NihalJain NihalJain closed this Nov 6, 2024
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