Re: [PR] Improve type safety of CollectionUtils.toTreeSet [dubbo]
zrlw merged PR #16306: URL: https://github.com/apache/dubbo/pull/16306 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Improve type safety of CollectionUtils.toTreeSet [dubbo]
skt-shinyruo commented on PR #16306: URL: https://github.com/apache/dubbo/pull/16306#issuecomment-4619972987 Hi @zrlw, RainYuY has approved the latest commit and all checks are passing. Could you please take another look when available? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Improve type safety of CollectionUtils.toTreeSet [dubbo]
skt-shinyruo commented on code in PR #16306:
URL: https://github.com/apache/dubbo/pull/16306#discussion_r3346483111
##
dubbo-common/src/main/java/org/apache/dubbo/common/utils/CollectionUtils.java:
##
@@ -429,6 +429,15 @@ public static Set toTreeSet(Set set) {
return set;
}
+public static Set toTreeSet(Set set, Comparator
comparator) {
+if (set == null) {
Review Comment:
The null check is intentional here. For this comparator overload, null and
empty inputs need different handling: null keeps the existing null-return
behavior, while an empty non-null set should still return an empty TreeSet
initialized with the provided comparator. Returning the original set from
isEmpty(set) would lose that comparator. The addAll call is a no-op for an
empty set.
I also added a null-input test for this overload in 630215ff94.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Improve type safety of CollectionUtils.toTreeSet [dubbo]
RainYuY commented on code in PR #16306:
URL: https://github.com/apache/dubbo/pull/16306#discussion_r3345536870
##
dubbo-common/src/main/java/org/apache/dubbo/common/utils/CollectionUtils.java:
##
@@ -419,7 +419,7 @@ public static T first(List values) {
return values.get(0);
}
-public static Set toTreeSet(Set set) {
+public static > Set toTreeSet(Set
set) {
Review Comment:
Have you confirmed that all element types for the toTreeSet calls in the
Dubbo codebase have implemented Comparable? This looks like an incompatible
change?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Improve type safety of CollectionUtils.toTreeSet [dubbo]
RainYuY commented on code in PR #16306:
URL: https://github.com/apache/dubbo/pull/16306#discussion_r3345536870
##
dubbo-common/src/main/java/org/apache/dubbo/common/utils/CollectionUtils.java:
##
@@ -419,7 +419,7 @@ public static T first(List values) {
return values.get(0);
}
-public static Set toTreeSet(Set set) {
+public static > Set toTreeSet(Set
set) {
Review Comment:
Have you confirmed that all element types for the toTreeSet calls in the
Dubbo codebase have implemented Comparable? This looks like an incompatible
change?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Improve type safety of CollectionUtils.toTreeSet [dubbo]
RainYuY commented on code in PR #16306:
URL: https://github.com/apache/dubbo/pull/16306#discussion_r3345532493
##
dubbo-common/src/main/java/org/apache/dubbo/common/utils/CollectionUtils.java:
##
@@ -429,6 +429,15 @@ public static Set toTreeSet(Set set) {
return set;
}
+public static Set toTreeSet(Set set, Comparator
comparator) {
+if (set == null) {
Review Comment:
Why did it become null instead of checking for empty?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Improve type safety of CollectionUtils.toTreeSet [dubbo]
codecov-commenter commented on PR #16306: URL: https://github.com/apache/dubbo/pull/16306#issuecomment-4601000607 ## [Codecov](https://app.codecov.io/gh/apache/dubbo/pull/16306?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report :x: Patch coverage is `0%` with `5 lines` in your changes missing coverage. Please review. :white_check_mark: Project coverage is 36.95%. Comparing base ([`b65573e`](https://app.codecov.io/gh/apache/dubbo/commit/b65573e5ca2ed81383fd2973e32cb3ed185a90a4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a5a64f9`](https://app.codecov.io/gh/apache/dubbo/commit/a5a64f9b1a002c4faefb85ea6cb7b113594b77ca?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)). | [Files with missing lines](https://app.codecov.io/gh/apache/dubbo/pull/16306?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [...org/apache/dubbo/common/utils/CollectionUtils.java](https://app.codecov.io/gh/apache/dubbo/pull/16306?src=pr&el=tree&filepath=dubbo-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fdubbo%2Fcommon%2Futils%2FCollectionUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vdXRpbHMvQ29sbGVjdGlvblV0aWxzLmphdmE=) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/dubbo/pull/16306?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | > :exclamation: There is a different number of reports uploaded between BASE (b65573e) and HEAD (a5a64f9). Click for more details. > > HEAD has 5 uploads less than BASE > >| Flag | BASE (b65573e) | HEAD (a5a64f9) | >|--|--|--| >|unit-tests-java21|1|0| >|unit-tests-java8|1|0| >|unit-tests-java25|1|0| >|unit-tests-java11|1|0| >|unit-tests-java17|1|0| > Additional details and impacted files ```diff @@ Coverage Diff @@ ##3.3 #16306 +/- ## = - Coverage 60.80% 36.95% -23.86% + Complexity1176611749 -17 = Files 1953 1952-1 Lines 8918889171 -17 Branches 1345413370 -84 = - Hits 5423532952-21283 - Misses2939151636+22245 + Partials 5562 4583 -979 ``` | [Flag](https://app.codecov.io/gh/apache/dubbo/pull/16306/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [integration-tests-java21](https://app.codecov.io/gh/apache/dubbo/pull/16306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `32.16% <0.00%> (+0.04%)` | :arrow_up: | | [integration-tests-java8](https://app.codecov.io/gh/apache/dubbo/pull/16306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `32.24% <0.00%> (+<0.01%)` | :arrow_up: | | [samples-tests-java21](https://app.codecov.io/gh/apache/dubbo/pull/16306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `32.17% <0.00%> (-0.02%)` | :arrow_down: | | [samples-tests-java8](https://app.codecov.io/gh/apache/dubbo/pull/16306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `29.86% <0.00%> (+<0.01%)` | :arrow_up: | | [unit-tests-java11](https://app.codecov.io/gh/apache/dubbo/pull/16306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | | [unit-tests-java17](https://app.codecov.io/gh/apache/dubbo/pull/16306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | | [unit-tests-java21](https://app.codecov.io/gh/apache/dubbo/pull/16306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | | [unit-tests-java25](https://app.codecov.io/gh/apache/dubbo/pull/16306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
