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

CSV-215 CSV Record mutability #21

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

nmahendru
Copy link

I want to push another change which I feel will also be useful for the community. I want to add a CSVRecordMutable class which had a constructor which accepts a CSVRecord object. So when we have a CSVRecordMutable object from it then we can edit individual columns using it.
I would be using this to write back my edited CSV file. My use case is to read a csv, mangle some columns, write back a new csv.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 94.219% when pulling 0d54fa0 on nmahendru:master into 299fdcc on apache:master.

@coveralls
Copy link

coveralls commented Aug 15, 2017

Coverage Status

Coverage decreased (-0.2%) to 94.242% when pulling cbfee0a on nmahendru:master into 299fdcc on apache:master.

@garydgregory
Copy link
Member

I do not like the mutability toggle. Our options, I think are:

  • add CSVRecord.put(int, Object)
  • add CSVRecord.put(String, Object)

and document that the CSVRecord class is now mutable. This is the simplest solution.

Alternatively, if we are hard core about maintaining CSVRecord as immutable, we can talk about that.

BUT, note that the fact that the current implementation is immutable is NOT documented, all we say in the CSVRecord Javadoc is: "A CSV record parsed from a CSV file."

@nmahendru
Copy link
Author

The toggle was just an option to keep immutability in some sense. But Making it mutable and documenting it looks better. Wan't another pull request ?

@garydgregory
Copy link
Member

Up to you. I do not think we've reached any kind of consensus on the dev ML though.

@nmahendru
Copy link
Author

I'll wait then.

@sgoeschl
Copy link

Personally I don't like the "mutability" toggle and would prefer to have a setter - basically I see two options to implement the setter

  1. an immutable setter returning a new CSVRecord
  2. a mutable setter which modifies the object

@stain
Copy link
Member

stain commented Feb 9, 2018

See also #25 instead. The withValue() setters does like @sgoeschl suggest and either return a new CSVRecord or mutates the current object. mutable() or immutable() or a CVFormat flag can be used to force either variant for efficiency reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants