You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by GitBox <gi...@apache.org> on 2021/07/28 11:00:38 UTC
[GitHub] [sling-org-apache-sling-scripting-sightly-runtime] kwin opened a new pull request #9: SLING-10679 wrap every object in a single item list
kwin opened a new pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/pull/9
--
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: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly-runtime] kwin commented on a change in pull request #9: SLING-10679 wrap every object in a single item list
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/pull/9#discussion_r678372186
##########
File path: src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java
##########
@@ -308,12 +306,7 @@ public static String toString(Object object) {
Iterable<Object> iterable = (Iterable<Object>) object;
return fromIterator(iterable.iterator());
}
- if (object instanceof String || object instanceof Number) {
- Collection<Object> list = new ArrayList<>();
- list.add(object);
- return list;
- }
- return Collections.emptyList();
+ return Collections.singletonList(object);
Review comment:
Modifying the other collections to return immutable lists consistently requiress quite some updates in the test (which currently compare with equals against a mutable list), therefore I would like to separate this from this extension. Changing the single element list to be immutable didn't require any test changes.
--
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: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly-runtime] kwin commented on a change in pull request #9: SLING-10679 wrap every object in a single item list
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/pull/9#discussion_r678372186
##########
File path: src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java
##########
@@ -308,12 +306,7 @@ public static String toString(Object object) {
Iterable<Object> iterable = (Iterable<Object>) object;
return fromIterator(iterable.iterator());
}
- if (object instanceof String || object instanceof Number) {
- Collection<Object> list = new ArrayList<>();
- list.add(object);
- return list;
- }
- return Collections.emptyList();
+ return Collections.singletonList(object);
Review comment:
Modifying the other collections to return immutable lists consistently requires quite some updates in the test (which currently compare with equals against a mutable list), therefore I would like to separate this from this extension. Changing the single element list to be immutable didn't require any test changes.
Also this would require further changes on `getIndex(...)` as this cannot work on an object wrapped by `Collection.unmodifiableCollection`
##########
File path: src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java
##########
@@ -308,12 +306,7 @@ public static String toString(Object object) {
Iterable<Object> iterable = (Iterable<Object>) object;
return fromIterator(iterable.iterator());
}
- if (object instanceof String || object instanceof Number) {
- Collection<Object> list = new ArrayList<>();
- list.add(object);
- return list;
- }
- return Collections.emptyList();
+ return Collections.singletonList(object);
Review comment:
Modifying the other collections to return immutable lists consistently requires quite some updates in the test (which currently compare with equals against a mutable list), therefore I would like to separate this from this extension. Changing the single element list to be immutable didn't require any test changes.
Also this would require further changes on `getIndex(...)` as this cannot work on an object wrapped by `Collections.unmodifiableCollection`
--
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: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly-runtime] sonarcloud[bot] commented on pull request #9: SLING-10679 wrap every object in a single item list
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/pull/9#issuecomment-888219085
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly-runtime] kwin commented on a change in pull request #9: SLING-10679 wrap every object in a single item list
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/pull/9#discussion_r678390823
##########
File path: src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java
##########
@@ -308,12 +306,7 @@ public static String toString(Object object) {
Iterable<Object> iterable = (Iterable<Object>) object;
return fromIterator(iterable.iterator());
}
- if (object instanceof String || object instanceof Number) {
- Collection<Object> list = new ArrayList<>();
- list.add(object);
- return list;
- }
- return Collections.emptyList();
+ return Collections.singletonList(object);
Review comment:
After having a second thought it wasn't that much effort and only one change in the unit test was necessary, therefore I committed those changes in https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/pull/9/commits/a16076eca5099ef4134f5fa90adb76a6af624df4.
--
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: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly-runtime] kwin commented on a change in pull request #9: SLING-10679 wrap every object in a single item list
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/pull/9#discussion_r678285787
##########
File path: src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java
##########
@@ -308,12 +306,7 @@ public static String toString(Object object) {
Iterable<Object> iterable = (Iterable<Object>) object;
return fromIterator(iterable.iterator());
}
- if (object instanceof String || object instanceof Number) {
- Collection<Object> list = new ArrayList<>();
- list.add(object);
- return list;
- }
- return Collections.emptyList();
+ return Collections.singletonList(object);
Review comment:
The empty list being returned before was also immutable.
--
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: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly-runtime] kwin commented on a change in pull request #9: SLING-10679 wrap every object in a single item list
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/pull/9#discussion_r678372186
##########
File path: src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java
##########
@@ -308,12 +306,7 @@ public static String toString(Object object) {
Iterable<Object> iterable = (Iterable<Object>) object;
return fromIterator(iterable.iterator());
}
- if (object instanceof String || object instanceof Number) {
- Collection<Object> list = new ArrayList<>();
- list.add(object);
- return list;
- }
- return Collections.emptyList();
+ return Collections.singletonList(object);
Review comment:
Modifying the other collections to return immutable lists consistently requires quite some updates in the test (which currently compare with equals against a mutable list), therefore I would like to separate this from this extension. Changing the single element list to be immutable didn't require any test changes.
--
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: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly-runtime] sonarcloud[bot] removed a comment on pull request #9: SLING-10679 wrap every object in a single item list
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/pull/9#issuecomment-888219085
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly-runtime] kwin merged pull request #9: SLING-10679 wrap every object in a single item list
Posted by GitBox <gi...@apache.org>.
kwin merged pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/pull/9
--
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: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly-runtime] Buuhuu commented on a change in pull request #9: SLING-10679 wrap every object in a single item list
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/pull/9#discussion_r678304613
##########
File path: src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java
##########
@@ -308,12 +306,7 @@ public static String toString(Object object) {
Iterable<Object> iterable = (Iterable<Object>) object;
return fromIterator(iterable.iterator());
}
- if (object instanceof String || object instanceof Number) {
- Collection<Object> list = new ArrayList<>();
- list.add(object);
- return list;
- }
- return Collections.emptyList();
+ return Collections.singletonList(object);
Review comment:
But not the `ArrayList` returned with the single String | Number.
--
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: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly-runtime] Buuhuu commented on a change in pull request #9: SLING-10679 wrap every object in a single item list
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/pull/9#discussion_r678282735
##########
File path: src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java
##########
@@ -308,12 +306,7 @@ public static String toString(Object object) {
Iterable<Object> iterable = (Iterable<Object>) object;
return fromIterator(iterable.iterator());
}
- if (object instanceof String || object instanceof Number) {
- Collection<Object> list = new ArrayList<>();
- list.add(object);
- return list;
- }
- return Collections.emptyList();
+ return Collections.singletonList(object);
Review comment:
While I agree with the change, I think it comes with a certain risk as it changes the returned collection from a modifiable to an unmodifiable one. Should we make that somehow clear to the consumers?
In general I think we should return unmodifiable collections. Can you change `fromIterator()` accordingly as well?
--
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: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly-runtime] sonarcloud[bot] commented on pull request #9: SLING-10679 wrap every object in a single item list
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/pull/9#issuecomment-888458956
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&metric=new_coverage&view=list) [92.9% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly-runtime&pullRequest=9&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org