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.

Sign up for GitHub

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

Jump to bottom

[Feature][Style] Enable spotless to manage imports #11458

Merged
merged 3 commits into from
Aug 13, 2022
Merged

[Feature][Style] Enable spotless to manage imports #11458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
854b5ef
[Feature][Style] Enable spotless to manage imports (#11457)
EricGao888 Aug 12, 2022
44457b2
Add license head
EricGao888 Aug 12, 2022
61b40f7
Update imports order
EricGao888 Aug 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions ...zeppelin/src/main/java/org/apache/dolphinscheduler/plugin/task/zeppelin/ZeppelinTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,26 @@

package org.apache.dolphinscheduler.plugin.task.zeppelin;

import com.fasterxml.jackson.databind.ObjectMapper;
import kong.unirest.Unirest;
import org.apache.dolphinscheduler.plugin.task.api.AbstractTaskExecutor;
import org.apache.dolphinscheduler.plugin.task.api.TaskConstants;
import org.apache.dolphinscheduler.plugin.task.api.TaskExecutionContext;
import org.apache.dolphinscheduler.plugin.task.api.parameters.AbstractParameters;
import org.apache.dolphinscheduler.spi.utils.DateUtils;
import org.apache.dolphinscheduler.spi.utils.JSONUtils;

import org.apache.zeppelin.client.ClientConfig;
import org.apache.zeppelin.client.NoteResult;
import org.apache.zeppelin.client.ParagraphResult;
import org.apache.zeppelin.client.Status;
import org.apache.zeppelin.client.ZeppelinClient;

import java.util.HashMap;
import java.util.List;
import java.util.Map;

import kong.unirest.Unirest;

import com.fasterxml.jackson.databind.ObjectMapper;

public class ZeppelinTask extends AbstractTaskExecutor {

Expand All @@ -52,7 +55,6 @@ public class ZeppelinTask extends AbstractTaskExecutor {
*/
private ZeppelinClient zClient;


/**
* constructor
*
Expand Down Expand Up @@ -121,7 +123,8 @@ public void handle() throws Exception {

resultContent = resultContentBuilder.toString();
} else {
final ParagraphResult paragraphResult = this.zClient.executeParagraph(noteId, paragraphId, zeppelinParamsMap);
final ParagraphResult paragraphResult =
this.zClient.executeParagraph(noteId, paragraphId, zeppelinParamsMap);
resultContent = paragraphResult.getResultInText();
status = paragraphResult.getStatus();
}
Expand Down
55 changes: 32 additions & 23 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
<description>Dolphin Scheduler is a distributed and easy-to-expand visual DAG workflow scheduling system, dedicated
to solving the complex dependencies in data processing, making the scheduling system out of the box for data
processing.</description>

<modules>
<module>dolphinscheduler-bom</module>
<module>dolphinscheduler-alert</module>
Expand All @@ -58,7 +58,7 @@
<module>dolphinscheduler-ui</module>
<module>dolphinscheduler-scheduler-plugin</module>
</modules>

<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
Expand Down Expand Up @@ -91,11 +91,11 @@
<docker.tag>${project.version}</docker.tag>
<docker.build.skip>true</docker.build.skip>
<docker.push.skip>true</docker.push.skip>

<python.sign.skip>true</python.sign.skip>
<skipDepCheck>true</skipDepCheck>
</properties>

<dependencyManagement>
<dependencies>
<dependency>
Expand Down Expand Up @@ -173,13 +173,13 @@
<artifactId>dolphinscheduler-spi</artifactId>
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>org.apache.dolphinscheduler</groupId>
<artifactId>dolphinscheduler-data-quality</artifactId>
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>org.apache.dolphinscheduler</groupId>
<artifactId>dolphinscheduler-python</artifactId>
Expand Down Expand Up @@ -260,7 +260,7 @@
<artifactId>dolphinscheduler-registry-mysql</artifactId>
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>org.apache.dolphinscheduler</groupId>
<artifactId>dolphinscheduler-scheduler-api</artifactId>
Expand All @@ -271,7 +271,7 @@
<artifactId>dolphinscheduler-scheduler-quartz</artifactId>
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>org.apache.dolphinscheduler</groupId>
<artifactId>dolphinscheduler-datasource-all</artifactId>
Expand All @@ -282,7 +282,7 @@
<artifactId>dolphinscheduler-datasource-api</artifactId>
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>org.apache.dolphinscheduler</groupId>
<artifactId>dolphinscheduler-task-api</artifactId>
Expand All @@ -298,7 +298,7 @@
<artifactId>dolphinscheduler-task-all</artifactId>
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>org.apache.dolphinscheduler</groupId>
<artifactId>dolphinscheduler-ui</artifactId>
Expand All @@ -310,9 +310,9 @@
<version>${project.version}</version>
</dependency>
</dependencies>

</dependencyManagement>

<dependencies>
<!--
NOTE: only development / test phase dependencies (scope = test / provided)
Expand Down Expand Up @@ -370,7 +370,7 @@
<scope>test</scope>
</dependency>
</dependencies>

<build>
<pluginManagement>
<plugins>
Expand All @@ -380,7 +380,7 @@
<version>${rpm-maven-plugion.version}</version>
<inherited>false</inherited>
</plugin>

<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
Expand All @@ -392,7 +392,7 @@
<testTarget>${java.version}</testTarget>
</configuration>
</plugin>

<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-release-plugin</artifactId>
Expand All @@ -401,13 +401,13 @@
<tagNameFormat>@{project.version}</tagNameFormat>
</configuration>
</plugin>

<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-assembly-plugin</artifactId>
<version>${maven-assembly-plugin.version}</version>
</plugin>

<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-javadoc-plugin</artifactId>
Expand All @@ -417,7 +417,7 @@
<failOnError>false</failOnError>
</configuration>
</plugin>

<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
Expand Down Expand Up @@ -512,7 +512,7 @@
</plugin>
</plugins>
</pluginManagement>

<plugins>
<plugin>
<groupId>org.owasp</groupId>
Expand Down Expand Up @@ -554,7 +554,7 @@
</dependency>
</dependencies>
</plugin>

<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
Expand Down Expand Up @@ -584,7 +584,7 @@
</dependency>
</dependencies>
</plugin>

<!-- jenkins plugin jacoco report-->
<plugin>
<groupId>org.jacoco</groupId>
Expand Down Expand Up @@ -648,13 +648,22 @@
<eclipse>
<file>style/spotless_dolphinscheduler_formatter.xml</file>
</eclipse>
<removeUnusedImports />
<importOrder>
<file>style/eclipse.importorder</file>
</importOrder>
<replaceRegex>
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member Author

@EricGao888 EricGao888 Aug 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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)

Copy link
Member

Choose a reason for hiding this comment

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

@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)

Wow. The issue has been 2 years...

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Can you identify the failure of spotless:check is because of wildcard imports or other code style issues?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kezhenxu94 Yes, I just double-checked it. To reproduce, you might change

dolphinscheduler/dolphinscheduler-task-plugin/dolphinscheduler-task-zeppelin/src/main/java/org/apache/dolphinscheduler/plugin/task/zeppelin/ZeppelinTask.java

Lines 33 to 35 in 68316f6

import java.util.HashMap;
import java.util.List;
import java.util.Map;
to import java.util.*; and run mvn spotless:check.

Copy link
Member

Choose a reason for hiding this comment

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

@kezhenxu94 Yes, I just double-checked it. To reproduce, you might change

dolphinscheduler/dolphinscheduler-task-plugin/dolphinscheduler-task-zeppelin/src/main/java/org/apache/dolphinscheduler/plugin/task/zeppelin/ZeppelinTask.java

Lines 33 to 35 in 68316f6

import java.util.HashMap;
import java.util.List;
import java.util.Map;

to import java.util.*; and run mvn 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kezhenxu94 Yes, I just double-checked it. To reproduce, you might change

dolphinscheduler/dolphinscheduler-task-plugin/dolphinscheduler-task-zeppelin/src/main/java/org/apache/dolphinscheduler/plugin/task/zeppelin/ZeppelinTask.java

Lines 33 to 35 in 68316f6

import java.util.HashMap;
import java.util.List;
import java.util.Map;

to import java.util.*; and run mvn 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?

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.
image

<name>Remove wildcard imports</name>
<searchRegex>import\s+[^\*\s]+\*;(\r\n|\r|\n)</searchRegex>
<replacement>$1</replacement>
</replaceRegex>
</java>
<pom>
<sortPom>
<encoding>UTF-8</encoding>
<nrOfIndentSpace>4</nrOfIndentSpace>
<keepBlankLines>true</keepBlankLines>
<indentBlankLines>true</indentBlankLines>
<indentBlankLines>false</indentBlankLines>
<indentSchemaLocation>true</indentSchemaLocation>
<spaceBeforeCloseEmptyElement>true</spaceBeforeCloseEmptyElement>
<sortModules>false</sortModules>
Expand Down Expand Up @@ -747,7 +756,7 @@
<url>https://github.com/apache/dolphinscheduler</url>
<tag>HEAD</tag>
</scm>

<profiles>
<profile>
<id>docker</id>
Expand Down
23 changes: 23 additions & 0 deletions style/eclipse.importorder
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#
# Licensed to the Apache Software Foundation (ASF) under one or more
# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
# The ASF licenses this file to You under the Apache License, Version 2.0
# (the "License"); you may not use this file except in compliance with
# the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
#Organize Import Order
0=org.apache.dolphinscheduler
1=org.apache
2=java
3=javax
4=org
5=com