-
Notifications
You must be signed in to change notification settings - Fork 274
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
CSV-216: Add mutator withValue() to CSVRecord #25
base: master
Are you sure you want to change the base?
Conversation
if the record is already mutable, they will return the current object, otherwise a copy will be made using private CSVMutableRecord subclass. The method mutable() and immutable() can be used to ensure either semantics.
as mutator functions are directly in CSVRecord
public CSVRecord withValue(String name, String value) { | ||
CSVRecord r = mutable(); | ||
r.put(name, value); | ||
return r; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that here the javadoc leaves mutability of the returned CSVRecord undefined, this implementation will in fact always return a mutable CSVRecord (as can be verified with isMutable()
)
If someone did record.withValue(1, "b").immutable()
it would cause two array copies. But record.withValue(1, "b").withValue(2, "c")
would just make one array copy (or 0 if withMutableRecords was set in format).
final modifiers to most methods ... not sure what CSVMutableRecord class is needed for now!
|
||
private static final long serialVersionUID = 1L; | ||
|
||
CSVMutableRecord(String[] values, Map<String, Integer> mapping, String comment, long recordNumber, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am considering if there's not much point in the CSVMutableRecord
class anymore, except in any hope of the compiler being clever due to the final methods and the inline isMutable()
constant.
An alternative is to add a private final boolean mutable
to CSVRecord
and make it final again - a variant of #21 from @nmahendru.
} | ||
|
||
final void put(final int index, String value) { | ||
values[index] = value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be possible to modify columns outside the length of values
? (Would cause multiple array resizes if done many times)
as .clone() will do -- at least until someone tries to do .withValue(x) in an out-of-range column
f66a839 adds warnings about Is this OK to merge? @garydgregory said he would have a look. |
I would to review today or Thursday. |
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. |
Hello @stain and all, Starting in version 1.10.0, you can use the method This might avoid the need for this PR. |
Addresses CSV-215 and CSV-216
I know mutable
CSVRecord
caused quite a discussion earlier.@garydgregory created a CSV-216 branch b23f963 adding a new
CSVMutableRecord
which could be enabled in the formatwithMutableRecords(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 toCSVMutableRecord
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:
By default the records from the parser are immutable (
.isMutable()
returnfalse
), and thewith
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 onisMutable()
.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 makeCSVRecord.comment
non-final and package-accessible toCSVMutableRecord
(or it would have to keep a duplicatecomment
field and override all the comment methods)(This would have been easier if
CSVRecord
was an interface and we could avoid subclassing.)