CSV-264: Added DuplicateHeaderMode for flexibility with header strictness.

[SethFalco]

Instead of only having a boolean true/false for how duplicate header values are handled, this uses an enum instead.

Previously the only possibilities were:

• true: To allow duplicates.
• false: To disallow duplicates, except empty cells which were allowed to be duplicates.

This pull request makes an enum with three options:

• ALLOW_ALL: To always allow duplicates. (Same as true previously.)
• ALLOW_EMPTY: To allow duplicates only if they're empty cells. (Same as false previously.)
• DISALLOW: To disallow all duplicates.

This provides a little more flexibility in the strictness for the parser, and makes what the options do clearer.

Jira Issue: https://issues.apache.org/jira/browse/CSV-264

[aherbert]

Thanks for the PR. You have removed some public API methods. This is not allowed and would fail binary compatibility tests. I have enabled these on the CI environments. If you rebase on master the errors should appear in the checks.

You can run:

mvn

on the project to see the full checks on the code.

The mvn clirr:check goal will fail and indicate the public methods that have been removed. These should be supported in addition to the new methods with the new duplicate header names enum.

[coveralls]

Coverage increased (+0.02%) to 98.327% when pulling 1be64a6 on SethFalco:CSV-264 into 46bae4b on apache:master.

[SethFalco]

Sorry about that, I rebased master and applied the changes requested.
I believe this should be good now, though I'll take a peek at the unsuccessful checks now.

I usually use git pull and git merge to obtain changes, I'm a bit new to using rebase however.
I'm not 100% sure if I was supposed to use git push --force-with-lease..., but the usual git push... was throwing warnings, please let me know if that's a problem.

[garydgregory]

You can force push to your branch all you want; -) we usually do not rewrite history for long lived branches like master and release.

[SethFalco]

Not sure if you guys are interested in merging this PR, but figured I'd fix the merge conflicts that appeared from recent commits.

[garydgregory]

@SethiPandi
Please rebase on master.
TY!
Gary

[SethFalco]

Just a heads up that this is rebased with master now!

[garydgregory]

@SethFalco
Please accept my apologies for the delay in further reviewing this PR.

I feel there is something important missing here:

• What happens/should happen for each enum value when you call CSVRecord.get(String).
• What happens/should happen for each enum value when you call CSVRecord.toMap().

For example, if duplicate headers are allowed, does the first column win? Should it? Does the last column win? Certainly, it should not be random or undocumented.

Also, would you mind rebasing on master please? I've recently deprecated the with methods in favor of a Builder. New settings should be added through the Builder, not with methods.

[SethFalco]

It's worth noting this change doesn't impact how data is resolved internally or how headers are referenced, but just how if duplicate empty headers are allowed. I can peek into the current behavior anyway and try to document it if it's not already.

Meanwhile, I've rebased with master.
I also squashed my commits to make rebasing a bit easier.

I also updated an exception to no longer recommend using one of the deprecated methods.

Edit: Sorry, forgot to add @ Deprecated to the getter.

[SethFalco]

I didn't see it documented, figured the easiest way to find out would be to just test it.
It currently uses the last column with the given name.

What happens/should happen for each enum value when you call CSVRecord.get(String).

A	A
1	2
3	4

List<CSVRecord> records = parser.getRecords();

for (CSVRecord record : records)
    System.out.println(record.get("A"));

// 2
// 4

What happens/should happen for each enum value when you call CSVRecord.toMap().

A	A	B	B
1	2	5	6
3	4	7	8

List<CSVRecord> records = parser.getRecords();

for (CSVRecord record : records) {
    Map<String, String> map = record.toMap();
    System.out.println(map);
}

// {A=2, B=6}
// {A=4, B=8}

I think we could add a docstring on CSVRecord#get and CSVRecord#toMap to clarify this.
We could also add something on CSVFormat#setDuplicateHeaderMode?

It might be a good idea to add test cases to cover this scenario, if it'll become documented behavior.

Mind if I do this in a separate PR?
I'd say it counts as a separate change from this one, so it'll keep things tidier.

[garydgregory]

@SethFalco
Thank you for the update. Please see my comments scattered throughout.
Gary

[SethFalco]

Sorry about the imports, I'm guessing that due to a setting in IntelliJ setting back when I was using that.
I've rebased with master and done the requested changes. (I hope!)

[garydgregory]

@SethFalco
TY, the PR looks good to me. I'd like feedback from the community as well.

[SethFalco]

Just rebased this to resolve merge conflicts.

I'd like feedback from the community as well.

Seems no one wants to comment on this. ^-^'

Any chance this could be merged soon, or still waiting on feedback? (Note, I'm in no rush.)

[kinow]

It sounds like a good fix for the issue described in JIRA. Only adds one more setting, giving more flexibility for users.

+1 and thanks for the contribution and patience @SethFalco !

-Bruno