commons-csv icon indicating copy to clipboard operation
commons-csv copied to clipboard

CSV-216: Add mutator withValue() to CSVRecord

Open stain opened this issue 7 years ago • 3 comments

Addresses CSV-215 and CSV-216

I know mutable CSVRecord caused quite a discussion earlier.

@garydgregory created a CSV-216 branch b23f963e8dd4ca553a233e653a6220d8e80db9e9 adding a new CSVMutableRecord which could be enabled in the format withMutableRecords(true)

I like that this means the records remain immutable unless explicitly turned on (hence a CSVRecord would be safe to pass around), but I had trouble with this as that required casting to CSVMutableRecord to actually mutate.

It is also a bit weird to only be able to mutate a record in the middle of a parsing iterator - rows are still "immutable". There is no way to remove/reorder/insert records other than doing it out of bands (e.g. construct a new List).

This pull request augments the branch to add mutator functions:

for (CSVRecord r : csvparser) {
  CSVRecord rSoup = r.withValue(4, "soup")
                         .withValue(5, "fish");
  // original r is untouched and can be used again
  CSVRecord rBeans = r.withValue(3, "beans");
  someList.add(r);
  someList.add(rSoup);
  someList.add(rBeans);
}

By default the records from the parser are immutable (.isMutable() return false), and the with functions create a clones on demand. The mutated records are mutable, but you can force either with .mutable() or .immutable() -- which would either clone or return itself depending on isMutable().

Setting .withMutableRecords(true) in the format can be used to avoid the initial array clone of .withValue.

The subclass CSVMutableRecord is now package-private and just short-circuits some of the above, as the mutator functions are always available.

For completeness I added .withComment() -- but I'm not sure about the current approach here as it means I have to make CSVRecord.comment non-final and package-accessible to CSVMutableRecord (or it would have to keep a duplicate comment field and override all the comment methods)

(This would have been easier if CSVRecord was an interface and we could avoid subclassing.)

stain avatar Feb 09 '18 16:02 stain

f66a839 adds warnings about .clone() according to discussion with Gilles @sebbASF et al.

Is this OK to merge? @garydgregory said he would have a look.

stain avatar Feb 20 '18 11:02 stain

I would to review today or Thursday.

garydgregory avatar Feb 20 '18 14:02 garydgregory

Thanks, @garydgregory -- let me know if you feel this is not mature enough; I can always leave this PR out and run a 1.3 RC as-is.

stain avatar Feb 26 '18 16:02 stain