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

Turn CSVRecord into a List #35

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@mina86
Copy link

commented Dec 3, 2018

CSVRecord implements get(int) and size() methods so it’s already pretty
much a list. However, because it does not implement the List interface,
users cannot take advantage of convenience methods such as subList nor
can they pass records around to methods accepting lists.

Make CSVRecord extend AbstractList so that it implements List interface.

Because AbstractList pravides iterator() implementation, this allows us
to delete said method (alongside toList() method) reducing amount of
code.

@coveralls

This comment has been minimized.

Copy link

commented Dec 3, 2018

Coverage Status

Coverage increased (+0.5%) to 91.819% when pulling 5d43929 on mina86:master into 546fd69 on apache:master.

Turn CSVRecord into a List
CSVRecord implements get(int) and size() methods so it’s already pretty
much a list.  However, because it does not implement the List interface,
users cannot take advantage of convenience methods such as subList nor
can they pass records around to methods accepting lists.

Make CSVRecord extend AbstractList so that it implements List interface.

Because AbstractList pravides iterator() implementation, this allows us
to delete said method (alongside toList() method) reducing amount of
code.

@mina86 mina86 force-pushed the mina86:master branch from 91749d1 to 9256af7 Dec 3, 2018

@garydgregory

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

@mina86 ,

Thank you for your patch. We cannot break binary compatibility within the 1.x release line. That means you cannot remove public APIs for example.

Gary

@mina86

This comment has been minimized.

Copy link
Author

commented Dec 6, 2018

@garydgregory
Copy link
Contributor

left a comment

This change feels pretty heavy. I wonder what others think.

@mina86

This comment has been minimized.

Copy link
Author

commented Dec 12, 2018

I wonder what others think.

Well, that’s not something I can answer…

@garydgregory

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2018

I will ask on the dev ML for opinions.

@@ -43,6 +43,7 @@
<action issue="CSV-208" type="fix" dev="ggregory" due-to="Jurrie Overgoor">Fix escape character for POSTGRESQL_TEXT and POSTGRESQL_CSV formats.</action>
<action issue="CSV-232" type="fix" dev="ggregory" due-to="Jurrie Overgoor, Gary Gregory">Site link "Source Repository" does not work.</action>
<action issue="CSV-234" type="add" dev="ggregory" due-to="Roberto Benedetti, Gary Gregory">Add support for java.sql.Clob.</action>
<action type="add" dev="mina86" due-to="Michal Nazarewicz">Turn CSVRecord into a List.</action>

This comment has been minimized.

Copy link
@kinow

kinow Dec 13, 2018

Member

I believe the dev part will be the committer ID

This comment has been minimized.

Copy link
@mina86

mina86 Dec 13, 2018

Author

Yeah, I wasn’t sure what to put there. Or whether I should have edit this file at all.

This comment has been minimized.

Copy link
@kinow

kinow Dec 13, 2018

Member

Not a problem, it's not well documented what contributors should do about changes.xml. You can leave this file unchanged, and a committer will add this line later with his/her ID, and the due-to entry as above 👍

@kinow

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

This looks OK to me. It would still be an Iterable, as AbstractList also implements it. As the data values are not streamed, but rather kept in the internal values[] array, I think there is no performance issues by changing it to AbstractList.

I have only one concern here. Before this change, users have a complete Iterable object, which they can pass around (indeed not as a List, but they can use the parent interface or use helper methods to convert to List/Collection/etc).

But I believe we have only get and size implemented from the List interface. So while users would now have a List object, I believe they would get UnsupportedOperationException for remove, addAll, add, clear, etc.

We could add documentation alerting users about potential issues of using it as a List, but I think this could still lead to unexpected issues reported, and probably the need to keep implementing more methods, or liaise and indoctrinate users on how to use CSVRecord.

For that I am -0 for this change (i.e. my code would still work as I'm using Iterable, and if my memory helps I won't have exceptions for a while, but if others are strongly keen to have this feature, then I'd be fine).

Hope that helps
Bruno

@kinow

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

By the way, the code is really neat @mina86, with good tests. Hope to see other contributions like this 👍

@mina86

This comment has been minimized.

Copy link
Author

commented Dec 13, 2018

But I believe we have only get and size implemented from the List interface. So while users would now have a List object, I believe they would get UnsupportedOperationException for remove, addAll, add, clear, etc.

While that’s true, this is exactly how the interface is defined:

The "destructive" methods contained in this interface, that is, the methods that modify the collection on which they operate, are specified to throw UnsupportedOperationException if this collection does not support the operation.

(this particular quote is from Collection’s documentation) and I would expect no one who has any experience with Java to be surprised at that. There are multiple examples of lists which cannot be modified: Collections.unmodifiableList turns modifiable list into unmodifiable one, Arrays.asList supports set but does not support add or remove. Even Apache Commons has UnmodifiableList for some reason.

@kinow

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

I think your arguments above are good @mina86. I may be too pessimistic on my point of view, thinking that users could believe they can modify CSVRecord while parsing a CSV file and writing to another CSV file.

But as I said, I don't oppose this change 🙂 the code is good, it has tests, follows the code standards, and has a practical use. So we just need to wait for others to chime in and see what they prefer.

@kgonia

This comment has been minimized.

Copy link

commented Dec 20, 2018

If I may add something from developer perspective. I recently used commons-csv and this feature would be very desire change. It will allow to use stream api and simplify writing clean code. In some cases using iterator complicates writing clean code especially when business logic depends on values from cell on specific index.

@mina86

This comment has been minimized.

Copy link
Author

commented Dec 21, 2018

FYI, you can do StreamSupport.stream(record.splitterator(), false) to convert CSVRecord into a stream though it’s less efficient in some cases (since the stream is unsized) than if CSVRecord was a List.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.