git.net

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [CSV] Records as Lists


Hi Gilles!

Sorry, just came back from a long holiday, speaking Portuguese only. Semantic, vocabulary, etc, in English is a bit laggy right now.


>>On Tue, 18 Dec 2018 07:22:00 +0000 (UTC), Bruno P. Kinoshita wrote:
>> From what I understood from the previous messages & discussion on
>> GitHub, it would be more convenient for users to be able to have a
>> List instead of an Iterable,
>
>Why "instead"?
>The patch makes the class a subclass of "List"; and "List"
>implements "Iterable".


That's right, I meant to say that it would be an AbstractList, and not only an Iterable any more.



>> However, I think we are delivering an Iterable that's fully capable

>> to be used as an Iterable now. Whereas the proposal would make it a
>> read-only list, as that returned from unmodifiableList,
>
>That's not what the patch does:
>https://docs.oracle.com/javase/7/docs/api/java/util/AbstractList.html


I think tis another semantic/communication issue. From the link above:

"Note that this implementation throws an UnsupportedOperationException unless remove(int index) or removeRange(int fromIndex, int toIndex) is overridden."


That's what I meant. By extending AbstractList, users could now call remove and other methods, that would result in the exception above (similarly to the list returned via Collections.unmodifiableList).



>How?

>[In the original code, "toList" is private.]

Oh, good point!!! I wrote the e-mail from memory, a few days after reading the code, and after just arriving at home. Mea culpa.


>> by accidentally
>> trying to reuse CSVRecord while reading from one input and writing to
>> an output stream.
>
>I don't understand this.


That was a contrived example. Say you read the records, transform the Iterable into a List, and add/remove/clear columns/rows while processing the CSVRecord (e.g. you could be using commons-text and other libs to filter/process the entries). Then you get these values and create a new CSV.

When users realize they now have a List instead, they could rely on the CSVRecord object, instead of creating new Lists, and get some runtime errors. Corner case, and - again - a contrived example.

But I'm not opposing the change, just don't see the need for using AbstractList (though I've been writing Python for the past couple of months, so my Java-fu isn't really sharp right now).


Cheers
Bruno

________________________________
From: Gilles <gilles@xxxxxxxxxxxxxxxxxxxxx>
To: dev@xxxxxxxxxxxxxxxxxx 
Sent: Wednesday, 19 December 2018 1:28 AM
Subject: Re: [CSV] Records as Lists



Hi.

On Tue, 18 Dec 2018 07:22:00 +0000 (UTC), Bruno P. Kinoshita wrote:
> From what I understood from the previous messages & discussion on
> GitHub, it would be more convenient for users to be able to have a
> List instead of an Iterable,

Why "instead"?
The patch makes the class a subclass of "List"; and "List"
implements "Iterable".

> or instead of having to call the
> #toList() or convert to a List in some other way.
> I commented in the pull request, that I don't think there would be a
> performance penalty in doing so (at least I don't think so, as the
> values are not streamed, but rather kept in the private values 
> array).
> However, I think we are delivering an Iterable that's fully capable
> to be used as an Iterable now. Whereas the proposal would make it a
> read-only list, as that returned from unmodifiableList,

That's not what the patch does:
  https://docs.oracle.com/javase/7/docs/api/java/util/AbstractList.html

> i.e. throwing
> exceptions for add/clear/etc operations.

The original code (using "Arrays.asList") could not do those 
operations:
  
https://docs.oracle.com/javase/7/docs/api/java/util/Arrays.html#asList(T...)

> In my opinion, I prefer to keep it as an Iterable, leave the toList
> method, as I think current users could be affected

How?
[In the original code, "toList" is private.]

> by accidentally
> trying to reuse CSVRecord while reading from one input and writing to
> an output stream.

I don't understand this.

Regards,
Gilles


> So I'm -0 for it.
> Bruno
>
>       From: sebb <sebbaz@xxxxxxxxx>
>  To: Commons Developers List <dev@xxxxxxxxxxxxxxxxxx>
>  Sent: Tuesday, 18 December 2018 12:24 AM
>  Subject: Re: [CSV] Records as Lists
>
> What is the use-case for using lists?
>
> On Thu, 13 Dec 2018 at 18:34, Gary Gregory <garydgregory@xxxxxxxxx> 
> wrote:
>>
>> Hi All,
>>
>> I am looking for opinions on turning a CSV record into a list, as 
>> opposed
>> to the minimal current implementation. There would be side-effects 
>> like a
>> record becoming writable instead of read-only as the current 
>> implementation.
>>
>> Memory footprint would also be a concern.
>>
>> Please see https://github.com/apache/commons-csv/pull/35
>>
>> Gary
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxx
> For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxx



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxx
For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxx

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxx
For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxx