commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benedikt Ritter <benerit...@gmail.com>
Subject Re: [VOTE] Release Commons CSV 1.0 based on RC1
Date Fri, 18 Jul 2014 20:05:38 GMT


Send from my mobile device

> Am 15.07.2014 um 20:07 schrieb Benedikt Ritter <britter@apache.org>:
> 
> Hello Emmanuel,
> 
> thanks for your thorough review. I've worked through your list. Here are my comments:
> 
> 
> 2014-07-12 19:02 GMT+02:00 Emmanuel Bourg <ebourg@apache.org>:
>> I took a fresh look at the API and here is my review based on the
>> Javadoc and the content of the site only:
>> 
>> - The Javadoc for CSVFormat mentions a parseFile() method, but this
>> method doesn't exist.
> 
> fixed
>  
>> 
>> - CSVFormat.DEFAULT states that empty lines are "allowed". Does it mean
>> the empty lines are ignored or returned as an empty record? EDIT:
>> Actually it's properly documented but I got confused by the style of the
>> <h3> section. It's rendered like a field header and I thought that was
>> the documentation of the next field.
> 
> fixed
>  
>> 
>> - CSVFormat.DEFAULT, EXCEL and RFC4180 mention
>> 'withRecordSeparator(CRLF)', but CRLF is not visible in the Javadoc.
> 
> fixed
>  
>> 
>> - CSVFormat.newFormat() doesn't tell the value of the other settings. I
>> assume it's based on the DEFAULT format, but that's not obvious.
> 
> fixed
>  
>> 
>> - There is with/getIgnoreEmptyHeaders() but unlike lines there is only
>> one header. So we should probably remove the 's'. That would be
>> consistent with skipHeaderRecord (no 's').
> 
> skipHeaderRecord refers to the header records as a whole (so it's singular). ignore empty
headers refers to header column values, so it's plural. I guess that makes sense.
>  
>> 
>> - I see an inconsistency with the CSVFormat.is*() methods. There is a
>> mix of is*ingEnabled() and is*ing(). If a native English speaker could
>> look at these methods and tell if they are ok that would be great. For
>> example I wonder if isNullHandling() should be turned into
>> isHandlingNull() or isNullHandled().
> 
> I've changed all methods to "is * ing". Now only isCommentingEnabled is left. I don't
know what to do with this. I'm still looking forward to comments from a native speaker :)
>  
>> 
>> - I'm not fond of the CSVFormat.get*() methods returning a boolean. For
>> example I'd prefer isHeaderRecordSkipped() instead of getSkipHeaderRecord().
> 
> fixed
>  
>> 
>> - CSVFormat.withRecordSeparator() could state explicitly it doesn't
>> affect the parsing (i.e., if configured with \r\n, an input file with \n
>> only will be properly parsed).
> 
> fixed
>  
>> 
>> - That may sound dumb but I see CSVFormat.withCommentStart() and
>> intuitively expect a withCommentStop() method, probably because my
>> programmer's mind thinks about a block comment of a programming
>> language, but that doesn't exist in the CSV format. Maybe
>> withCommentMarker() would be less confusing.
> 
> fixed
>  
>> 
>> - looking at the Javadoc of CSVPrinter I don't understand how
>> printRecords() works and how it differs from printRecord() (same
>> signatures).
> 
> I've tried to clarify the JavaDoc of the said methods. Can you please review?
>  
>> 
>> - Does CSVPrinter.printRecord(ResultSet) print a header line derived
>> from the metadata? I would add a boolean parameter to control this.
> 
> No it doesn't. How about adding this in 1.1? I've created CSV-123 for this.
>  
>> 
>> - CSVPrinter has a println() method, this sparks a doubt: do I have to
>> call this after each printRecord()? An example in the class
>> documentation could clear that.
> 
> I've added a hint in the JavaDoc of printRecord.
>  
>> 
>> - The documentation for CSVRecord.isConsistent() could be improved. I
>> would rewrite the first sentence as "Tells if the record size matches
>> the header size". This is what will be displayed in the method table and
>> better conveys the purpose of this method than "Returns true if this
>> record is consistent", which is somewhat obvious for isConsistent() :)
> 
> fixed
>  
>> 
>> - The documentation for CSVRecord.toString() could specify the output
>> format.
> 
> Currently we're just calling Arrays.toString with the values, which could be improved.
I've created CSV-124 for this.
>  
>> 
>> - The documentation of CSVRecord.getRecordNumber() may elaborate on the
>> difference between line number and record number. A reference to
>> CSVParser.getCurrentLineNumber() would be useful.
> 
> fixed
>  
>> 
>> - CSVParser.getRecords(T records) doesn't look useful to me, I'd remove
>> it and suggest using records.addAll(parser.getRecords()) instead.
> 
> Agreed, I've removed the method.
>  
>> 
>> - Quote vs QuotePolicy: "Quote" is shorter but "QuotePolicy" is more
>> explicit, I'm not sure which one I prefer.
> 
> I neither, so I leave it as it is :o)

While looking at CSVFormat again, I noticed that the methods currently are withQuotePolicy
and getQuotePolicy. Now I'm feeling we should go with the more verbose but more explicit name
for that enum.

>  
>> - The <p> elements after <h2> create an excessive space in the Javadoc,
>> I suggest removing them.
> 
> Removing the <p> does only remove the space *after* the tag. The excessive space
before is the result of the <h2> tags.
>  
>> 
>> - The code examples in the userguide use a two spaces indentation :)
> 
> fixed
>  
>> 
>> - The documentation of CSVFormat could be copied in the userguide to
>> better show how to manipulate the API.
> 
> I don't like having the same content in two places. I've added an additional link to
the website. Anyway, the website can easily bu updated after the release ;-)
>  
>> 
>> 
>> That looks like a lot of comments by I'm really pleased with the API,
>> congratulations everyone I'm glad we are near a release.
> 
> Me too! I'll create RC2 now.
>  
>> 
>> Emmanuel Bourg
>> 
>> 
>> ---------------------------------------------------------------------
>> 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, 7-Bit, 0 bytes)
View raw message