You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/08/09 14:06:52 UTC
[GitHub] [commons-csv] sman-81 opened a new pull request, #249: [CSV-300] Return new ArrayList in CSVRecord.toList() to prevent access to private values array
sman-81 opened a new pull request, #249:
URL: https://github.com/apache/commons-csv/pull/249
This PR offers a fix to bug CSV-300.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] sman-81 commented on pull request #249: [CSV-300] Return new ArrayList in CSVRecord.toList() to prevent access to private values array
Posted by GitBox <gi...@apache.org>.
sman-81 commented on PR #249:
URL: https://github.com/apache/commons-csv/pull/249#issuecomment-1210211580
Thanks for your feedback. PR updated.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] kinow commented on a diff in pull request #249: [CSV-300] Return new ArrayList in CSVRecord.toList() to prevent access to private values array
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #249:
URL: https://github.com/apache/commons-csv/pull/249#discussion_r941754150
##########
src/main/java/org/apache/commons/csv/CSVRecord.java:
##########
@@ -298,13 +299,14 @@ public Stream<String> stream() {
}
/**
- * Converts the values to a List.
+ * Converts the values to a new {@code List}.<br>
+ * Modifications to the list do not alter this record.
*
- * @return a new List
+ * @return a new {@code List}
* @since 1.9.0
*/
public List<String> toList() {
- return Arrays.asList(values);
+ return new ArrayList<>(Arrays.asList(values));
Review Comment:
See @aherbert 's comment in the JIRA linked about returning an immutable list. Unless there's a reason for allowing users to modify the returned list, I think an immutable list would be best :+1:
Thanks for the pull request!
-Bruno
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on pull request #249: [CSV-300] Return new ArrayList in CSVRecord.toList() to prevent access to private values array
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #249:
URL: https://github.com/apache/commons-csv/pull/249#issuecomment-1209829807
-1 needs a unit test
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on pull request #249: [CSV-300] Return new ArrayList in CSVRecord.toList() to prevent access to private values array
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #249:
URL: https://github.com/apache/commons-csv/pull/249#issuecomment-1210382217
https://issues.apache.org/jira/browse/CSV-300
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a diff in pull request #249: [CSV-300] Return new ArrayList in CSVRecord.toList() to prevent access to private values array
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #249:
URL: https://github.com/apache/commons-csv/pull/249#discussion_r941766996
##########
src/main/java/org/apache/commons/csv/CSVRecord.java:
##########
@@ -298,13 +299,14 @@ public Stream<String> stream() {
}
/**
- * Converts the values to a List.
+ * Converts the values to a new {@code List}.<br>
+ * Modifications to the list do not alter this record.
*
- * @return a new List
+ * @return a new {@code List}
* @since 1.9.0
*/
public List<String> toList() {
- return Arrays.asList(values);
+ return new ArrayList<>(Arrays.asList(values));
Review Comment:
I don't see the point in returning a half-functional list. The list should not mangle the underlying record that's all.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] kinow commented on a diff in pull request #249: [CSV-300] Return new ArrayList in CSVRecord.toList() to prevent access to private values array
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #249:
URL: https://github.com/apache/commons-csv/pull/249#discussion_r941764572
##########
src/main/java/org/apache/commons/csv/CSVRecord.java:
##########
@@ -298,13 +299,14 @@ public Stream<String> stream() {
}
/**
- * Converts the values to a List.
+ * Converts the values to a new {@code List}.<br>
+ * Modifications to the list do not alter this record.
*
- * @return a new List
+ * @return a new {@code List}
* @since 1.9.0
*/
public List<String> toList() {
- return Arrays.asList(values);
+ return new ArrayList<>(Arrays.asList(values));
Review Comment:
(Also needs to update the javadocs as mentioned in the JIRA comment too, and needs unit tests as pointed below by @garydgregory :+1: )
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory closed pull request #249: [CSV-300] Return new ArrayList in CSVRecord.toList() to prevent access to private values array
Posted by GitBox <gi...@apache.org>.
garydgregory closed pull request #249: [CSV-300] Return new ArrayList in CSVRecord.toList() to prevent access to private values array
URL: https://github.com/apache/commons-csv/pull/249
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] codecov-commenter commented on pull request #249: [CSV-300] Return new ArrayList in CSVRecord.toList() to prevent access to private values array
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #249:
URL: https://github.com/apache/commons-csv/pull/249#issuecomment-1209802959
# [Codecov](https://codecov.io/gh/apache/commons-csv/pull/249?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#249](https://codecov.io/gh/apache/commons-csv/pull/249?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4ffdf29) into [master](https://codecov.io/gh/apache/commons-csv/commit/532e08c3c4b7b8e8d71002480bf4976f9a8985f8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (532e08c) will **not change** coverage.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #249 +/- ##
=========================================
Coverage 96.99% 96.99%
Complexity 529 529
=========================================
Files 11 11
Lines 1166 1166
Branches 205 205
=========================================
Hits 1131 1131
Misses 23 23
Partials 12 12
```
| [Impacted Files](https://codecov.io/gh/apache/commons-csv/pull/249?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...rc/main/java/org/apache/commons/csv/CSVRecord.java](https://codecov.io/gh/apache/commons-csv/pull/249/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY3N2L0NTVlJlY29yZC5qYXZh) | `100.00% <100.00%> (ø)` | |
:mega: Codecov can now indicate which changes are the most critical in Pull Requests. [Learn more](https://about.codecov.io/product/feature/runtime-insights/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on pull request #249: [CSV-300] Return new ArrayList in CSVRecord.toList() to prevent access to private values array
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #249:
URL: https://github.com/apache/commons-csv/pull/249#issuecomment-1210388725
Hm, I don't see the point of a read only list. IMO if I convert the record to a list, I should be able to do whatever I want with the list. If the list is RO, then I have to make a copy to edit it which is counter intuitive and wasteful. This is a toList() convert method and not a getList() access method after all. The point of a list here IMO is that I can edit its size, unlike an array.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] sman-81 commented on pull request #249: [CSV-300] Return new ArrayList in CSVRecord.toList() to prevent access to private values array
Posted by GitBox <gi...@apache.org>.
sman-81 commented on PR #249:
URL: https://github.com/apache/commons-csv/pull/249#issuecomment-1210512471
Reason for closing pr: @garydgregory implemented his own solution in master, making my pr obsolete.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] sman-81 closed pull request #249: [CSV-300] Return new ArrayList in CSVRecord.toList() to prevent access to private values array
Posted by GitBox <gi...@apache.org>.
sman-81 closed pull request #249: [CSV-300] Return new ArrayList in CSVRecord.toList() to prevent access to private values array
URL: https://github.com/apache/commons-csv/pull/249
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org