phoenix icon indicating copy to clipboard operation
phoenix copied to clipboard

DRAFT: Code formatting using spotless in Phoenix

Open NihalJain opened this issue 1 year ago • 5 comments

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

NihalJain avatar Oct 06 '24 06:10 NihalJain

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.

NihalJain avatar Oct 06 '24 07:10 NihalJain

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.

stoty avatar Oct 09 '24 11:10 stoty

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.

lfrancke avatar Oct 18 '24 11:10 lfrancke

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.

NihalJain avatar Oct 18 '24 14:10 NihalJain

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

lfrancke avatar Oct 18 '24 14:10 lfrancke

work superceded at https://github.com/apache/phoenix/pull/2023

NihalJain avatar Nov 06 '24 16:11 NihalJain