You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "panbingkun (via GitHub)" <gi...@apache.org> on 2023/05/16 10:11:46 UTC
[GitHub] [spark] panbingkun opened a new pull request, #41185: [SPARK-43525][BUILD] Enhance ImportOrderChecker rules for `group.scala`
panbingkun opened a new pull request, #41185:
URL: https://github.com/apache/spark/pull/41185
### What changes were proposed in this pull request?
- The pr aims to enhance ImportOrderChecker rules for `group.scala`
- Adjust the code import order according to the above rules.
### Why are the changes needed?
I found that some developers sometimes write 'collection.JavaConverters._' when import `JavaConverters._`, while others write 'scala.JavaConverters._'.
Actually, they all belong to the `scala group`, so it is necessary for us to specify a clearer rule to make the code style more consistent
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Pass GA.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon closed pull request #41185: [SPARK-43525][BUILD] Import `scala.collection` instead of `collection`
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #41185: [SPARK-43525][BUILD] Import `scala.collection` instead of `collection`
URL: https://github.com/apache/spark/pull/41185
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #41185: [SPARK-43525][BUILD] Import `scala.collection` instead of `collection`
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #41185:
URL: https://github.com/apache/spark/pull/41185#issuecomment-1550037989
Could you re-trigger the failed pipeline, @panbingkun ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #41185: [SPARK-43525][BUILD] Import `scala.collection` instead of `collection`
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41185:
URL: https://github.com/apache/spark/pull/41185#issuecomment-1550560512
late LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #41185: [SPARK-43525][BUILD] Import using `scala.collection.JavaConverters` instead of `collection.JavaConverters`
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #41185:
URL: https://github.com/apache/spark/pull/41185#discussion_r1195112050
##########
connector/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisInputDStream.scala:
##########
@@ -17,9 +17,9 @@
package org.apache.spark.streaming.kinesis
+import collection.JavaConverters._
Review Comment:
Is it feasible to add a `IllegalImportsChecker` item to `scalastyle-config.xml` like:
```
<check level="error" class="org.scalastyle.scalariform.IllegalImportsChecker" enabled="true">
<parameters><parameter name="illegalImports"><![CDATA[collection.]]></parameter></parameters>
<customMessage>Please use scala.collection instead.</customMessage>
</check>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #41185: [SPARK-43525][BUILD] Import using `scala.collection` instead of `collection`
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #41185:
URL: https://github.com/apache/spark/pull/41185#discussion_r1195364002
##########
scalastyle-config.xml:
##########
@@ -346,6 +346,11 @@ This file is divided into 3 sections:
]]></customMessage>
</check>
+ <check level="error" class="org.scalastyle.scalariform.IllegalImportsChecker" enabled="true">
Review Comment:
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #41185: [SPARK-43525][BUILD] Import `scala.collection` instead of `collection`
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #41185:
URL: https://github.com/apache/spark/pull/41185#issuecomment-1550518666
Merged to master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #41185: [SPARK-43525][BUILD] Enhance ImportOrderChecker rules for `group.scala`
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #41185:
URL: https://github.com/apache/spark/pull/41185#discussion_r1194983128
##########
connector/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisInputDStream.scala:
##########
@@ -17,9 +17,9 @@
package org.apache.spark.streaming.kinesis
+import collection.JavaConverters._
Review Comment:
Can we just write it as scala.collection.*, and disallow other cases? Would be great to double check Scala guidance for this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] panbingkun commented on a diff in pull request #41185: [SPARK-43525][BUILD] Import using `scala.collection.JavaConverters` instead of `collection.JavaConverters`
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #41185:
URL: https://github.com/apache/spark/pull/41185#discussion_r1195126026
##########
connector/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisInputDStream.scala:
##########
@@ -17,9 +17,9 @@
package org.apache.spark.streaming.kinesis
+import collection.JavaConverters._
Review Comment:
1.I have checked the specifications of scala and there are no clear regulations.
2.At the same time, I searched the scala source code and found that in the `2.12.17` version,
the number of import writing methods in the form of `import scala.collection.JavaConverters._` is 37, while the number of import writing methods in the form of `import collection.JavaConverters._` is 3.
So, it is reasonable for us to prohibit importing in the form of `import collection.JavaConverters._` here.
3.In the `2.13.8` version, the number of import writing methods in the form of `import scala.collection.JavaConverters._` is 3, while the number of import writing methods in the form of `import collection.JavaConverters._` is 1.
According to official documents, in the 2.13 version, `collection.JavaConverters` has already been deprecated, so we need to consider this issue in the future.
<img width="789" alt="image" src="https://github.com/apache/spark/assets/15246973/1e759bbd-f066-4f96-92f0-db7875f6f084">
link: https://docs.scala-lang.org/overviews/core/collections-migration-213.html#deprecated-things-in-212-that-have-been-removed-in-213
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] panbingkun commented on a diff in pull request #41185: [SPARK-43525][BUILD] Import using `scala.collection.JavaConverters` instead of `collection.JavaConverters`
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #41185:
URL: https://github.com/apache/spark/pull/41185#discussion_r1195126026
##########
connector/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisInputDStream.scala:
##########
@@ -17,9 +17,9 @@
package org.apache.spark.streaming.kinesis
+import collection.JavaConverters._
Review Comment:
1.I have checked the specifications of scala and there are no clear regulations.
2.At the same time, I searched the scala source code and found that in the `2.12.17` version,
the number of import writing methods in the form of `import scala.collection.JavaConverters._` is 37, while the number of import writing methods in the form of `import collection.JavaConverters._` is 3.
**So, it is reasonable for us to prohibit importing in the form of `import collection.JavaConverters._` here.**
3.In the `2.13.8` version, the number of import writing methods in the form of `import scala.collection.JavaConverters._` is 3, while the number of import writing methods in the form of `import collection.JavaConverters._` is 1.
According to official documents, in the 2.13 version, `collection.JavaConverters` has already been deprecated, so we need to consider this issue in the future.
<img width="789" alt="image" src="https://github.com/apache/spark/assets/15246973/1e759bbd-f066-4f96-92f0-db7875f6f084">
link: https://docs.scala-lang.org/overviews/core/collections-migration-213.html#deprecated-things-in-212-that-have-been-removed-in-213
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] panbingkun commented on a diff in pull request #41185: [SPARK-43525][BUILD] Import using `scala.collection.JavaConverters` instead of `collection.JavaConverters`
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #41185:
URL: https://github.com/apache/spark/pull/41185#discussion_r1195128418
##########
connector/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisInputDStream.scala:
##########
@@ -17,9 +17,9 @@
package org.apache.spark.streaming.kinesis
+import collection.JavaConverters._
Review Comment:
> Is it feasible to add a `IllegalImportsChecker` item to `scalastyle-config.xml` like:
>
> ```
> <check level="error" class="org.scalastyle.scalariform.IllegalImportsChecker" enabled="true">
> <parameters><parameter name="illegalImports"><![CDATA[collection.]]></parameter></parameters>
> <customMessage>Please use scala.collection instead.</customMessage>
> </check>
> ```
Yes, that's right. Currently, this solution is more reasonable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] panbingkun commented on pull request #41185: [SPARK-43525][BUILD] Enhance ImportOrderChecker rules for `group.scala`
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #41185:
URL: https://github.com/apache/spark/pull/41185#issuecomment-1549387264
cc @srowen
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org