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

[Feature #861][ALL] Remove useless imports #862

Merged
merged 3 commits into from
Dec 24, 2021
Merged

[Feature #861][ALL] Remove useless imports #862

merged 3 commits into from
Dec 24, 2021

Conversation

wuchunfu
Copy link
Member

Purpose of this pull request

close #861

Check list

@xleoken
Copy link
Member

xleoken commented Dec 24, 2021

hi @wuchunfu, can we add some scala checkstyle rule to check it?

@CalvinKirs
Copy link
Member

hi @wuchunfu, can we add some scala checkstyle rule to check it?

We have a test, but I don’t know why it didn’t take effect this time. Let me try again later.

@CalvinKirs
Copy link
Member

hi @wuchunfu, can we add some scala checkstyle rule to check it?

We have a test, but I don’t know why it didn’t take effect this time. Let me try again later.

I saw it, he only checked java, I will modify it later

@CalvinKirs
Copy link
Member

hi @wuchunfu, can we add some scala checkstyle rule to check it?

We have a test, but I don’t know why it didn’t take effect this time. Let me try again later.

I saw it, he only checked java, I will modify it later

@wuchunfu Or can you add the changes here in your pr?

@wuchunfu
Copy link
Member Author

hi @wuchunfu, can we add some scala checkstyle rule to check it?

We have a test, but I don’t know why it didn’t take effect this time. Let me try again later.

I saw it, he only checked java, I will modify it later

@wuchunfu Or can you add the changes here in your pr?

Okay, I will add a scalastyle.xml to check it later.

@wuchunfu
Copy link
Member Author

hi @wuchunfu, can we add some scala checkstyle rule to check it?

We have a test, but I don’t know why it didn’t take effect this time. Let me try again later.

I saw it, he only checked java, I will modify it later

@wuchunfu Or can you add the changes here in your pr?

Okay, I will add a scalastyle.xml to check it later.

I think I should submit another pr because there may be more changes after adding scalastyle.xml .

@CalvinKirs
Copy link
Member

hi @wuchunfu, can we add some scala checkstyle rule to check it?

We have a test, but I don’t know why it didn’t take effect this time. Let me try again later.

I saw it, he only checked java, I will modify it later

@wuchunfu Or can you add the changes here in your pr?

Okay, I will add a scalastyle.xml to check it later.

I think I should submit another pr because there may be more changes after adding scalastyle.xml .

well, It's better

@wuchunfu
Copy link
Member Author

cc @CalvinKirs

plugin: BaseSparkTransform,
ds: Dataset[Row]): Unit = {
plugin: BaseSparkTransform,
ds: Dataset[Row]): Unit = {
Copy link
Member

@xleoken xleoken Dec 24, 2021

Choose a reason for hiding this comment

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

the tab size seems a little long.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok i will fix it, thx

@@ -80,9 +74,7 @@ object SparkBatchExecution {
ds.createOrReplaceTempView(tableName)
}

private[seatunnel] def registerInputTempView(
Copy link
Member

Choose a reason for hiding this comment

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

Just a nit: I think we can keep it as it is, when the function has many args, the first form is better.
I am not sure if others prefer to the second form.

private[seatunnel] def registerInputTempView(
      source: BaseSparkSource[Dataset[Row]],
      environment: SparkEnvironment): Unit = {
private[seatunnel] def registerInputTempView(source: BaseSparkSource[Dataset[Row]], environment: SparkEnvironment): Unit = {

Copy link
Member Author

Choose a reason for hiding this comment

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

Do I want to restore it? I want to hear your suggestions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also feel that the first one will be better.

Copy link
Member

@CalvinKirs CalvinKirs left a comment

Choose a reason for hiding this comment

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

LGTM

@CalvinKirs CalvinKirs merged commit fbfe751 into apache:dev Dec 24, 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.

[Feature][ALL] Remove useless imports
3 participants