commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Emmanuel Bourg <ebo...@apache.org>
Subject Re: [VOTE] Release Commons CSV 1.0 based on RC1
Date Sat, 12 Jul 2014 17:02:49 GMT
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.

- 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.

- CSVFormat.DEFAULT, EXCEL and RFC4180 mention
'withRecordSeparator(CRLF)', but CRLF is not visible in the Javadoc.

- 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.

- 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').

- 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'm not fond of the CSVFormat.get*() methods returning a boolean. For
example I'd prefer isHeaderRecordSkipped() instead of getSkipHeaderRecord().

- 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).

- 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.

- looking at the Javadoc of CSVPrinter I don't understand how
printRecords() works and how it differs from printRecord() (same
signatures).

- Does CSVPrinter.printRecord(ResultSet) print a header line derived
from the metadata? I would add a boolean parameter to control 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.

- 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() :)

- The documentation for CSVRecord.toString() could specify the output
format.

- The documentation of CSVRecord.getRecordNumber() may elaborate on the
difference between line number and record number. A reference to
CSVParser.getCurrentLineNumber() would be useful.

- CSVParser.getRecords(T records) doesn't look useful to me, I'd remove
it and suggest using records.addAll(parser.getRecords()) instead.

- Quote vs QuotePolicy: "Quote" is shorter but "QuotePolicy" is more
explicit, I'm not sure which one I prefer.

- The <p> elements after <h2> create an excessive space in the Javadoc,
I suggest removing them.

- The code examples in the userguide use a two spaces indentation :)

- The documentation of CSVFormat could be copied in the userguide to
better show how to manipulate the API.


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.

Emmanuel Bourg


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


Mime
View raw message