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 <[email protected]>: > 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: [email protected] > For additional commands, e-mail: [email protected] > > -- http://people.apache.org/~britter/ http://www.systemoutprintln.de/ http://twitter.com/BenediktRitter http://github.com/britter
