commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benedikt Ritter <brit...@apache.org>
Subject Re: svn commit: r1592371 - in /commons/proper/csv/trunk/src: main/java/org/apache/commons/csv/CSVParser.java test/java/org/apache/commons/csv/CSVParserTest.java
Date Mon, 05 May 2014 05:24:00 GMT
Good morning sebb,


2014-05-04 23:36 GMT+02:00 sebb <sebbaz@gmail.com>:

> On 4 May 2014 17:22,  <britter@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 ;-)

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. For now, this is no reason for a veto IMHO.

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-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>


-- 
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message