Skip to content

Add eclipse formatter in maven spotless plugin#6807

Closed
mqliang wants to merge 3 commits intoapache:masterfrom
mqliang:fmt-enforce
Closed

Add eclipse formatter in maven spotless plugin#6807
mqliang wants to merge 3 commits intoapache:masterfrom
mqliang:fmt-enforce

Conversation

@mqliang
Copy link
Contributor

@mqliang mqliang commented Apr 16, 2021

Description

Add eclipse formatter in maven spotless plugin. "org.eclipse.jdt.core.compiler.source" should be set as 1.8 otherwise spotless plugin will panic.

cc @Jackie-Jiang @fx19880617

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

  • Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

  • Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

  • New configuration options
  • Deprecation of configurations
  • Signature changes to public methods/interfaces
  • New plugins added or old plugins removed
  • Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Documentation

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

A lot of changes are not in sync with the intellij format. I think most of the developpers are using intellij to format the code. Let's try to make the plug-in the same behavior as auto-formatting with intellij

@codecov-commenter
Copy link

Codecov Report

Merging #6807 (184089b) into master (cb9889b) will decrease coverage by 7.92%.
The diff coverage is 65.03%.

❗ Current head 184089b differs from pull request most recent head ad61432. Consider uploading reports for the commit ad61432 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master    #6807      +/-   ##
============================================
- Coverage     74.00%   66.08%   -7.93%     
  Complexity       12       12              
============================================
  Files          1414     1414              
  Lines         68886    68639     -247     
  Branches       9942     9935       -7     
============================================
- Hits          50979    45360    -5619     
- Misses        14569    20048    +5479     
+ Partials       3338     3231     -107     
Flag Coverage Δ Complexity Δ
integration ? ?
unittests 66.08% <65.03%> (+0.09%) 12.00 <0.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...org/apache/pinot/broker/api/RequestStatistics.java 44.11% <0.00%> (-21.04%) 0.00 <0.00> (ø)
...e/pinot/broker/api/resources/PinotBrokerDebug.java 0.00% <ø> (-76.48%) 0.00 <0.00> (ø)
...t/broker/api/resources/PinotBrokerHealthCheck.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...pinot/broker/api/resources/PinotBrokerRouting.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...pinot/broker/api/resources/PinotClientRequest.java 0.00% <0.00%> (-34.43%) 0.00 <0.00> (ø)
...not/broker/broker/helix/ClusterChangeMediator.java 73.62% <ø> (-1.10%) 0.00 <0.00> (ø)
...che/pinot/broker/queryquota/MaxHitRateTracker.java 95.45% <ø> (ø) 0.00 <0.00> (ø)
...thandler/SingleConnectionBrokerRequestHandler.java 12.76% <0.00%> (-72.66%) 0.00 <0.00> (ø)
...outing/segmentselector/SegmentSelectorFactory.java 60.00% <ø> (ø) 0.00 <0.00> (ø)
.../main/java/org/apache/pinot/client/Connection.java 36.36% <0.00%> (-8.09%) 0.00 <0.00> (ø)
... and 851 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb9889b...ad61432. Read the comment docs.

@mqliang
Copy link
Contributor Author

mqliang commented Apr 16, 2021

A lot of changes are not in sync with the intellij format. I think most of the developpers are using intellij to format the code. Let's try to make the plug-in the same behavior as auto-formatting with intellij

The level of difficulty is extremely high:

  • Intellj is not an open source project, the formatter it used is built-in, so there is no intelllj-formater maven plugin. Which means we can not use intellj-code-style.xml directly.
  • The inability to work out a set of eclipse formatting rules that are equivalent to what we use in IntelliJ basically prevents us from using the eclipse plugin (the one we use in this PR).
  • IntelliJ can import eclipse code styles, but translation is not accurate enough, and the opposite direction is not supported. Imtellj can not export formatting rules as eclipse formatting rules neither. So we are unable to generate eclipse rules in an automatic way.

So, what we can do is either:

  • Keep the import order and unused imports removal check we made in Add spotless maven plugin to enforce java format. #6782, but give up the code block auto-formatting. Let engineer manually format the file (github workflow can only detect violation at the imports section, code-block part violation can not be detected).

Or

  • Forget about the IDE formatting, engineer don't need to manually format the file using IDE, but manually run mvn spotless:format to format (github workflow will fail and prompt engineer to run mvn spotless:format if there are any violations).

Option 2 can force the whole codebase formatted in a consistent way, but not match the intellij auto-formatting behavior.

@Jackie-Jiang
Copy link
Contributor

@mqliang
I would suggest doing the followings:

  1. Change the eclipse format to follow the current intellij format as much as possible (e.g. new line for exception, new space for array declaration etc.). We should not see almost all files reformatted
  2. Reformat the code base using the plug-in
  3. Remove the intellij format and always stick with the eclipse format
  4. Update the Code Setup page: https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup

@mqliang
Copy link
Contributor Author

mqliang commented Apr 17, 2021

@Jackie-Jiang

Change the eclipse format to follow the current intellij format as much as possible (e.g. new line for exception, new space for array declaration etc.). We should not see almost all files reformatted

For "new line for exception", in Intellj, it's corresponding option is called "always wrap throws keyword":
Jietu20210416-234504

Unfortunately, Eclipse formatter does not has such a option, here is a complete list of Eclipse formatter options: https://github.com/eclipse/eclipse.jdt.core/blob/7e35bbd8b9cc36a449e562c61841f9968ec4bec3/org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/DefaultCodeFormatterOptions.java#L122-L524

For new space for array declaration etc, I have change the codestyle-eclipse.xml file to match IntellJ behavior.

So, it's almost impossible to make the maven plugin be consistent with IntellJ behavior if we use eclipse formatter given that IntellJ/Eclipse provide different options.

If we wanner keep consistency across IntellJ IDE, Eclipse IDE and maven plugin, the only choice is the google formatter, which has plugin for both IntellJ/Eclipse/Maven.

The drawback is:

Note: There is no configurability as to the formatter's algorithm for formatting. This is a deliberate design decision to unify 
our code formatting on a single format.

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Seems the most changes are caused by putting throws on the same line. I'm okay with it if there is no workaround.
In general, we want to keep minimal changes to the current code and migrate to the eclipse formatter.
Please announce this change in the pinot-dev channel and schedule a time to merge this change. The changes in this PR could cause several conflicts, and contributors should be notified so that they can rebase the changes accordingly.

_brokerConf.addProperty(Broker.CONFIG_OF_BROKER_ID, _brokerId);
}

public static HelixBrokerStarter getDefault() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change the formatter to not force the order of the methods?

@mqliang mqliang closed this Oct 14, 2021
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