You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by sebb <se...@gmail.com> on 2014/05/04 23:36:55 UTC
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
On 4 May 2014 17:22, <br...@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
> + }
> 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
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
Posted by sebb <se...@gmail.com>.
On 5 May 2014 06:24, Benedikt Ritter <br...@apache.org> wrote:
> Good morning sebb,
>
>
> 2014-05-04 23:36 GMT+02:00 sebb <se...@gmail.com>:
>
>> On 4 May 2014 17:22, <br...@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-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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
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
Posted by Benedikt Ritter <br...@apache.org>.
Good morning sebb,
2014-05-04 23:36 GMT+02:00 sebb <se...@gmail.com>:
> On 4 May 2014 17:22, <br...@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