[Feature][Style] Enable spotless to manage imports #11458
Conversation
EricGao888
requested review from
kezhenxu94,
caishunfeng,
SbloodyS and
zhuangchong
as code owners
style/eclipse.importorder
Outdated
| 0=java | ||
| 1=javax | ||
| 2=org | ||
| 3=com |
There was a problem hiding this comment.
FYI, we could not have an empty line at the end of this file because it would cause Spotless to fail to parse the imports management rules.
There was a problem hiding this comment.
If we hope to manage the import order, it's better to use the origin order
org.apache.dolphinscheduler,org.apache,java,javax,org,com
otherwise, the default order is still ok.
There was a problem hiding this comment.
If we hope to manage the import order, it's better to use the origin order
org.apache.dolphinscheduler,org.apache,java,javax,org,comotherwise, the default order is still ok.
Sure, I will give a try. If not work, will recover it.
There was a problem hiding this comment.
@ruanwenjun Updated. It works well.
|
Kudos, SonarCloud Quality Gate passed! |
| <importOrder> | ||
| <file>style/eclipse.importorder</file> | ||
| </importOrder> | ||
| <replaceRegex> |
There was a problem hiding this comment.
This removes the star import. But will it instead replaces the star import with the used specific class import? If not, this will cause compiling failed. Can you change a class with this example?
There was a problem hiding this comment.
@kezhenxu94 diffplug/spotless#649 (comment) It is hardly possible for Spotless to replace with wildcards with specific imports, thus this is some kind of workaround actually. To prevent developers from pushing the commits without knowing that wildcards has been removed by their pre-commit hook, one possible solution is to add a message in pre-commit hook to inform them of this behavior. WDYT?
There was a problem hiding this comment.
@kezhenxu94 FYI, I also added a comment in the origin issue to seek help to see if there is some good practice to handle wildcard imports with Spotless: diffplug/spotless#649 (comment)
There was a problem hiding this comment.
@kezhenxu94 FYI, I also added a comment in the origin issue to seek help to see if there is some good practice to handle
wildcard importswithSpotless: diffplug/spotless#649 (comment)
Wow. The issue has been 2 years...
There was a problem hiding this comment.
This removes the star import. But will it instead replaces the star import with the used specific class import? If not, this will cause compiling failed. Can you change a class with this example?
@kezhenxu94 Just a follow-up, as I've tested it locally, if we use mvn spotless:check instead of mvn spotless:apply, it only blocks the wildcard imports without removing them. To improve, we could change the pre-commit hook from mvn spotless:apply to mvn spotless:check and add an output message to inform contributors that they should remove wildcard imports. WDYT?
There was a problem hiding this comment.
This removes the star import. But will it instead replaces the star import with the used specific class import? If not, this will cause compiling failed. Can you change a class with this example?
@kezhenxu94 Just a follow-up, as I've tested it locally, if we use
mvn spotless:checkinstead ofmvn spotless:apply, it only blocks thewildcard importswithout removing them. To improve, we could change the pre-commit hook frommvn spotless:applytomvn spotless:checkand add an output message to inform contributors that they should removewildcard imports. WDYT?
Can you identify the failure of spotless:check is because of wildcard imports or other code style issues?
There was a problem hiding this comment.
@kezhenxu94 Yes, I just double-checked it. To reproduce, you might change
import java.util.*; and run mvn spotless:check.
There was a problem hiding this comment.
@kezhenxu94 Yes, I just double-checked it. To reproduce, you might change
to
import java.util.*;and runmvn spotless:check.
How can yo tell the spotless:check failure is because of wildcard import, if I have other code style issues and no wildcard import?
There was a problem hiding this comment.
@kezhenxu94 Yes, I just double-checked it. To reproduce, you might change
to
import java.util.*;and runmvn spotless:check.How can yo tell the
spotless:checkfailure is because of wildcard import, if I have other code style issues and no wildcard import?
Good question, since each rule has its name, I think there might be a way to configure Spotless to explicitly tell which rule the code fails. I will take a look and see if there is a way to do so.
Purpose of the pull request
imports.Brief change log
Spotlessto remove unusedimports.Spotlessto sortimportsin better order (instead of simply sorting import alphabetically).Spotlessto remove wildcards imports.Spotlessto remove empty line indents inpom.xml.ZeppelinTask.javaas an example.Verify this pull request