[jira] [Commented] (CSV-42) Lots of possible changes
[
https://issues.apache.org/jira/browse/CSV-42?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13691947#comment-13691947
]
Gary Gregory commented on CSV-42:
-
Just a note on:
{quote}
Added a close method. The try-with-resources statement in Java 7 makes
resources management much easier, there is no need to add a close() method to
the parser.
{quote}
The try-with-resources in Java 7 requires the a class implements Closeable,
which means implementing close(), but the parser does not create closeable
resources, it takes a {{Reader}}, which itself can be used with
try-with-resources in Java 7.
> Lots of possible changes
>
>
> Key: CSV-42
> URL: https://issues.apache.org/jira/browse/CSV-42
> Project: Commons CSV
> Issue Type: Improvement
>Affects Versions: 1.0
>Reporter: Bob Smith
>Priority: Minor
> Fix For: 1.0
>
> Attachments: src.zip
>
>
> I made a lot of changes to pretty much all of the classes in the csv package.
> I thought it would be better to put all of the the changes here in one
> issue, but feel free to only take the parts you like (if any). Hopefully if
> nothing else the test cases will be useful to you.
> I'll try to list most of the changes here, but I'm sure I'm forgetting some.
> This should include all of the big changes at least. I focused mostly on the
> parser, but I also made a few changes to the printer classes (although I
> don't think I added any new test cases there).
> h3. General Changes
> - Changed all class names with "CSV" in them to use "Csv". This is how it
> appears in the commons-lang "escapeCsv" methods and I think it's easier to
> read the class name when acronyms are not in all upper case. (x) {color:red}3
> letter acronyms are usually kept in uppercase (for example URLConnection or
> SAXParser in the JDK, but there are some exceptions){color}
> - Formatted the code. I used Eclipse with a version of the Java formatting
> style that uses spaces instead of tabs and with a few other small changes to
> try to make it more similar to the style of this code. The formatting was
> inconsistent before (sometimes 2 space indent, sometimes 4) which made it
> really hard to work on. (/) {color:green}DONE{color}
> - Removed all deprecated methods/constructors (/) {color:green}DONE{color}
> - Made all public classes final. If there is ever a need to create subclasses
> of them then this could be changed, but I think it would be better to at
> least start them as final (since once they are released as non-final it's
> hard to go back).
> - A few bug fixes (and test cases for them)
> h3. CsvParser
> - There were a few bugs for special cases, so I made as small of changes as I
> could to the parser code to fix these.
> - Added a lot of test cases. I created a test case for all bugs that I found,
> so even if you don't use my changes to this class you should be able to use
> the test cases to find all of the same bugs.
> - Added a close method. (x) {color:red} The try-with-resources statement in
> Java 7 makes resources management much easier, there is no need to add a
> close() method to the parser.{color}
> - Renamed the nextValue method to getValue (so it is more consistent with the
> getAll and getLine method names). I think I would prefer to use a different
> method name prefix for all three of these (like "readAll") since I wouldn't
> normally expect a "get" method to have side effects, but I didn't want to
> just change the names of the most used methods. (x) {color:red}This method
> has been removed, the parser now works line by line.{color}
> - Changed the getLineNumber method to return the correct line number when
> there are multi-line values. (x) {color:red}The suggested code counts the
> number of records instead of the number of lines. For debugging it's better
> to return the actual line number.{color}
> - Moved all of the lexer methods into an inner CsvLexer class that is
> completely independent of the CsvParser class. The methods were already
> separated out, so it wasn't a very big change. I also moved the lexer test
> cases into a new CsvLexerTest class. (/) {color:green}DONE{color}
> - Got rid of the interpreting unicode escape options. This doesn't really
> have anything to do with parsing a CSV file so I think it should be left up
> to the user of the class to implement this if needed. As an example, I made a
> CsvParserUnicodeEscapeTest class that uses the code from the lexer in a
> Reader subclass. One nice thing is that with this implementation, the
> interpreted values can be used as the delimiter, encapsulator, etc. (/)
> {color:green}DONE - The unicode unescaping is now handled by a class
> implementing java.io.Reader (to be contributed to Commons IO).{color}
> - Got rid of the "escape" option
[jira] [Commented] (CSV-42) Lots of possible changes
[
https://issues.apache.org/jira/browse/CSV-42?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13691912#comment-13691912
]
Benedikt Ritter commented on CSV-42:
Let's review this once again an decide what we want to change for 1.0
> Lots of possible changes
>
>
> Key: CSV-42
> URL: https://issues.apache.org/jira/browse/CSV-42
> Project: Commons CSV
> Issue Type: Improvement
>Affects Versions: 1.0
>Reporter: Bob Smith
>Priority: Minor
> Fix For: 1.0
>
> Attachments: src.zip
>
>
> I made a lot of changes to pretty much all of the classes in the csv package.
> I thought it would be better to put all of the the changes here in one
> issue, but feel free to only take the parts you like (if any). Hopefully if
> nothing else the test cases will be useful to you.
> I'll try to list most of the changes here, but I'm sure I'm forgetting some.
> This should include all of the big changes at least. I focused mostly on the
> parser, but I also made a few changes to the printer classes (although I
> don't think I added any new test cases there).
> h3. General Changes
> - Changed all class names with "CSV" in them to use "Csv". This is how it
> appears in the commons-lang "escapeCsv" methods and I think it's easier to
> read the class name when acronyms are not in all upper case. (x) {color:red}3
> letter acronyms are usually kept in uppercase (for example URLConnection or
> SAXParser in the JDK, but there are some exceptions){color}
> - Formatted the code. I used Eclipse with a version of the Java formatting
> style that uses spaces instead of tabs and with a few other small changes to
> try to make it more similar to the style of this code. The formatting was
> inconsistent before (sometimes 2 space indent, sometimes 4) which made it
> really hard to work on. (/) {color:green}DONE{color}
> - Removed all deprecated methods/constructors (/) {color:green}DONE{color}
> - Made all public classes final. If there is ever a need to create subclasses
> of them then this could be changed, but I think it would be better to at
> least start them as final (since once they are released as non-final it's
> hard to go back).
> - A few bug fixes (and test cases for them)
> h3. CsvParser
> - There were a few bugs for special cases, so I made as small of changes as I
> could to the parser code to fix these.
> - Added a lot of test cases. I created a test case for all bugs that I found,
> so even if you don't use my changes to this class you should be able to use
> the test cases to find all of the same bugs.
> - Added a close method. (x) {color:red} The try-with-resources statement in
> Java 7 makes resources management much easier, there is no need to add a
> close() method to the parser.{color}
> - Renamed the nextValue method to getValue (so it is more consistent with the
> getAll and getLine method names). I think I would prefer to use a different
> method name prefix for all three of these (like "readAll") since I wouldn't
> normally expect a "get" method to have side effects, but I didn't want to
> just change the names of the most used methods. (x) {color:red}This method
> has been removed, the parser now works line by line.{color}
> - Changed the getLineNumber method to return the correct line number when
> there are multi-line values. (x) {color:red}The suggested code counts the
> number of records instead of the number of lines. For debugging it's better
> to return the actual line number.{color}
> - Moved all of the lexer methods into an inner CsvLexer class that is
> completely independent of the CsvParser class. The methods were already
> separated out, so it wasn't a very big change. I also moved the lexer test
> cases into a new CsvLexerTest class. (/) {color:green}DONE{color}
> - Got rid of the interpreting unicode escape options. This doesn't really
> have anything to do with parsing a CSV file so I think it should be left up
> to the user of the class to implement this if needed. As an example, I made a
> CsvParserUnicodeEscapeTest class that uses the code from the lexer in a
> Reader subclass. One nice thing is that with this implementation, the
> interpreted values can be used as the delimiter, encapsulator, etc. (/)
> {color:green}DONE - The unicode unescaping is now handled by a class
> implementing java.io.Reader (to be contributed to Commons IO).{color}
> - Got rid of the "escape" option for the same reason as the unicode escape
> option. I replaced it with an encapsulator escape option that is only used as
> an escape operator on the encapsulator character.
> h3. ExtendedBufferedReader
> - Greatly simplified this class. I removed all the methods that weren't being
> used (including keeping track of the line number) and changed the lookahead
> o
