On 5 May 2014 06:24, Benedikt Ritter <brit...@apache.org> wrote: > Good morning sebb, > > > 2014-05-04 23:36 GMT+02:00 sebb <seb...@gmail.com>: > >> On 4 May 2014 17:22, <brit...@apache.org> wrote: >> > Author: britter >> > Date: Sun May 4 16:22:34 2014 >> > New Revision: 1592371 >> > >> > URL: http://svn.apache.org/r1592371 >> > Log: >> > CSV-112: HeaderMap is inconsistent when it is parsed from an input with >> duplicate columns names. Reported by Romain Gossé >> > >> > Modified: >> > >> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/CSVParser.java >> > >> commons/proper/csv/trunk/src/test/java/org/apache/commons/csv/CSVParserTest.java >> > >> > Modified: >> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/CSVParser.java >> > URL: >> http://svn.apache.org/viewvc/commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/CSVParser.java?rev=1592371&r1=1592370&r2=1592371&view=diff >> > >> ============================================================================== >> > --- >> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/CSVParser.java >> (original) >> > +++ >> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/CSVParser.java >> Sun May 4 16:22:34 2014 >> > @@ -29,6 +29,7 @@ import java.io.StringReader; >> > import java.net.URL; >> > import java.nio.charset.Charset; >> > import java.util.ArrayList; >> > +import java.util.Arrays; >> > import java.util.Collection; >> > import java.util.Iterator; >> > import java.util.LinkedHashMap; >> > @@ -368,6 +369,9 @@ public final class CSVParser implements >> > // build the name to index mappings >> > if (header != null) { >> > for (int i = 0; i < header.length; i++) { >> > + if (hdrMap.containsKey(header[i])) { >> > + throw new IllegalStateException("The header >> contains duplicate names: " + Arrays.toString(header)); >> >> -1: >> >> I think that is thw wrong exception - should probably be >> IllegalArgumentException >> > > > It's good to be reminded that you're reviewing commits closeley by your > occasional -1 ;-)
Actually it was the JIRA that alerted me. > In fact I had the code throw IAE at first, but then I saw that CSVFormat > also throws ISE. If we change this part of the code the other has to be > changed as well. Yes, there seem to be a lot of incorrect ISE throws. One method which uses both - correctly I think - is CSVRecord#get(String). I think all the ISEs/IAEs need to be reviewed. > For now, this is no reason for a veto IMHO. Perhaps a veto was overkill, I don't want the code to be released with the wrong exception. It may be tricky to change it later. > Feel free to change both, the parser and the format. > > Benedikt > > >> >> > + } >> > hdrMap.put(header[i], Integer.valueOf(i)); >> > } >> > } >> > >> > Modified: >> commons/proper/csv/trunk/src/test/java/org/apache/commons/csv/CSVParserTest.java >> > URL: >> http://svn.apache.org/viewvc/commons/proper/csv/trunk/src/test/java/org/apache/commons/csv/CSVParserTest.java?rev=1592371&r1=1592370&r2=1592371&view=diff >> > >> ============================================================================== >> > --- >> commons/proper/csv/trunk/src/test/java/org/apache/commons/csv/CSVParserTest.java >> (original) >> > +++ >> commons/proper/csv/trunk/src/test/java/org/apache/commons/csv/CSVParserTest.java >> Sun May 4 16:22:34 2014 >> > @@ -492,6 +492,11 @@ public class CSVParserTest { >> > parser.close(); >> > } >> > >> > + @Test(expected = IllegalStateException.class) >> > + public void testDuplicateHeaderEntries() throws Exception { >> > + CSVParser.parse("a,b,a\n1,2,3\nx,y,z", >> CSVFormat.DEFAULT.withHeader(new String[]{})); >> > + } >> > + >> > @Test >> > public void testGetLine() throws IOException { >> > final CSVParser parser = CSVParser.parse(CSV_INPUT, >> CSVFormat.DEFAULT.withIgnoreSurroundingSpaces(true)); >> > >> > >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> >> > > > -- > http://people.apache.org/~britter/ > http://www.systemoutprintln.de/ > http://twitter.com/BenediktRitter > http://github.com/britter --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
