Send from my mobile device
> Am 15.07.2014 um 20:07 schrieb Benedikt Ritter <[email protected]>: > > 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) 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: [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
