Mailing-List: contact dev-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Commons Developers List" <dev@commons.apache.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Fri, 28 Oct 2016 20:25:34 +0200
From: Gilles <gilles@harfang.homelinux.org>
To: <dev@commons.apache.org>
Subject: Re: commons-csv git commit: [CSV-201] Do not use RuntimeException
 in CSVParser.iterator().new Iterator() {...}.getNextRecord(). Use an
 IllegalStateException instead (KISS for now, no need for a custom
 exception)
In-Reply-To: <CACZkXPzn6PeMmHMjXyw35OS=+52u6iJhFDcMaTFLPWovqw6=_g@mail.gmail.com>
References: <60131c1261f94c4aa9bdbd0c21fa7be1@git.apache.org>
 <CAB917RK+EZ4iNvst2b2ZH66GLx2DMG+wg30YEVPGHC-SY7AQRA@mail.gmail.com>
 <CAB917RLtWe8Tbmb52SGHOj_8pLZHNgzsrQF4rc3MC4JAWvTwow@mail.gmail.com>
 <CACZkXPziP2RtBvC0g9pFjigD-5xbBfQ_wbTBuRk0Hsup+Q5GDg@mail.gmail.com>
 <CACZkXPyAMchR71kTNywbxJKeTNQ+tPpKwRCKKSTD2si37BC+zA@mail.gmail.com>
 <CACZkXPzn6PeMmHMjXyw35OS=+52u6iJhFDcMaTFLPWovqw6=_g@mail.gmail.com>
Message-ID: <74401a307b7ccd46822f2555c7b27d2b@scarlet.be>
User-Agent: Scarlet Webmail
X-DCC-scarlet.be-Metrics: hel; whitelist
X-Virus-Scanned: clamav-milter 0.98.1-exp at hel
X-Virus-Status: Clean
archived-at: Fri, 28 Oct 2016 18:25:47 -0000

On Fri, 28 Oct 2016 08:03:58 -0700, Gary Gregory wrote:
> I do not think reverting is right since the previous is obviously 
> bogus, to
> me, that is. Throwing a RE is never useful, again IMO. We should just
> evolve the code from here.
>
> Would you like to outline choices?
>
> 1) NSEE
> 2) Custom
> 3) ISE
> 4) ?

A custom exception inheriting from ISE (as previously suggested)
would combine the advantages of both 2) and 3).

Gilles

>
> Gary (will be offline most of today)
>
> On Oct 28, 2016 5:36 AM, "Benedikt Ritter" <britter@apache.org> 
> wrote:
>
> Hello,
>
> Benedikt Ritter <britter@apache.org> schrieb am Mi., 26. Okt. 2016 um
> 21:38 Uhr:
>
>> Hello,
>>
>> <ggregory@apache.org> schrieb am Di., 25. Okt. 2016 um 22:59 Uhr:
>>
>> Repository: commons-csv
>> Updated Branches:
>>   refs/heads/master b2a50d4a3 -> 1c7a9910b
>>
>>
>> [CSV-201] Do not use RuntimeException in CSVParser.iterator().new
>> Iterator() {...}.getNextRecord(). Use an IllegalStateException 
>> instead
>> (KISS for now, no need for a custom exception)
>>
>> Project: http://git-wip-us.apache.org/repos/asf/commons-csv/repo
>> Commit: 
>> http://git-wip-us.apache.org/repos/asf/commons-csv/commit/1c7a9910
>> Tree: 
>> http://git-wip-us.apache.org/repos/asf/commons-csv/tree/1c7a9910
>> Diff: 
>> http://git-wip-us.apache.org/repos/asf/commons-csv/diff/1c7a9910
>>
>> Branch: refs/heads/master
>> Commit: 1c7a9910be857a4f33ed216701407806e1d20670
>> Parents: b2a50d4
>> Author: Gary Gregory <ggregory@apache.org>
>> Authored: Tue Oct 25 13:59:32 2016 -0700
>> Committer: Gary Gregory <ggregory@apache.org>
>> Committed: Tue Oct 25 13:59:32 2016 -0700
>>
>> 
>> ----------------------------------------------------------------------
>>  src/changes/changes.xml                             |  1 +
>>  src/main/java/org/apache/commons/csv/CSVParser.java | 16 
>> ++++++++++------
>>  2 files changed, 11 insertions(+), 6 deletions(-)
>> 
>> ----------------------------------------------------------------------
>>
>>
>>
>> http://git-wip-us.apache.org/repos/asf/commons-csv/blob/
> 1c7a9910/src/changes/changes.xml
>> 
>> ----------------------------------------------------------------------
>> diff --git a/src/changes/changes.xml b/src/changes/changes.xml
>> index d5d8c8a..e49265b 100644
>> --- a/src/changes/changes.xml
>> +++ b/src/changes/changes.xml
>> @@ -43,6 +43,7 @@
>>        <action issue="CSV-193" type="fix" dev="ggregory" 
>> due-to="Matthias
>> Wiehl">Fix incorrect method name 'withFirstRowAsHeader' in user
>> guide.</action>
>>        <action issue="CSV-171" type="fix" dev="ggregory" 
>> due-to="Gary
>> Gregory, Michael Graessle, Adrian Bridgett">Negative numeric values 
>> in the
>> first column are always quoted in minimal mode.</action>
>>        <action issue="CSV-187" type="update" dev="ggregory" 
>> due-to="Gary
>> Gregory">Update platform requirement from Java 6 to 7.</action>
>> +      <action issue="CSV-201" type="update" dev="ggregory"
>> due-to="Benedikt Ritter, Gary Gregory">Do not use RuntimeException 
>> in
>> CSVParser.iterator().new Iterator() {...}.getNextRecord()</action>
>>        <action issue="CSV-189" type="add" dev="ggregory" 
>> due-to="Peter
>> Holzwarth, Gary Gregory">CSVParser: Add factory method accepting
>> InputStream.</action>
>>        <action issue="CSV-190" type="add" dev="ggregory" 
>> due-to="Gary
>> Gregory">Add convenience API CSVFormat.print(File, Charset)</action>
>>        <action issue="CSV-191" type="add" dev="ggregory" 
>> due-to="Gary
>> Gregory">Add convenience API CSVFormat.print(Path, Charset)</action>
>>
>>
>> http://git-wip-us.apache.org/repos/asf/commons-csv/blob/
> 1c7a9910/src/main/java/org/apache/commons/csv/CSVParser.java
>> 
>> ----------------------------------------------------------------------
>> diff --git a/src/main/java/org/apache/commons/csv/CSVParser.java
>> b/src/main/java/org/apache/commons/csv/CSVParser.java
>> index f5829cc..410e6a4 100644
>> --- a/src/main/java/org/apache/commons/csv/CSVParser.java
>> +++ b/src/main/java/org/apache/commons/csv/CSVParser.java
>> @@ -501,10 +501,14 @@ public final class CSVParser implements
>> Iterable<CSVRecord>, Closeable {
>>      /**
>>       * Returns an iterator on the records.
>>       *
>> -     * <p>IOExceptions occurring during the iteration are wrapped 
>> in a
>> -     * RuntimeException.
>> -     * If the parser is closed a call to {@code next()} will throw 
>> a
>> -     * NoSuchElementException.</p>
>> +     * <p>
>> +     * An {@link IOException} caught during the iteration are 
>> re-thrown
>> as an
>> +     * {@link IllegalStateException}.
>> +     * </p>
>> +     * <p>
>> +     * If the parser is closed a call to {@link Iterator#next()} 
>> will
>> throw a
>> +     * {@link NoSuchElementException}.
>> +     * </p>
>>       */
>>      @Override
>>      public Iterator<CSVRecord> iterator() {
>> @@ -515,8 +519,8 @@ public final class CSVParser implements
>> Iterable<CSVRecord>, Closeable {
>>                  try {
>>                      return CSVParser.this.nextRecord();
>>                  } catch (final IOException e) {
>> -                    // TODO: This is not great, throw an ISE 
>> instead?
>> -                    throw new RuntimeException(e);
>> +                    throw new IllegalStateException(
>> +                            e.getClass().getSimpleName() + " 
>> reading next
>> record: " + e.toString(), e);
>>
>>
>> As discussed in CSV-201, I don't think this fix is complete. We 
>> should
>> rather throw a custom RuntimeException to make it easy for calling 
>> code to
>> react to exactly this problem.
>>
>
> I think this needs to be reverted until we have consensus about how 
> to
> solve this issue.
>
>
>>
>> Benedikt
>>
>>
>>                  }
>>              }
>>
>>
>>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org