commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benedikt Ritter <brit...@apache.org>
Subject Re: [VOTE] Release Commons CSV 1.0 based on RC1
Date Tue, 15 Jul 2014 18:07:55 GMT
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)


> - 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, None, 0 bytes)
View raw message