> You have added test data for CSVFormat for 1.7 and 1.8 and these do not work (commented out). I take it this means serialization has been broken since the CSVFormat.delimiter was changed from char to String in 1.9.0.
That's correct, Alex. I added the comments for documentation. Should we decide to fix serialization from version 1.7 and version 1.8 upwards, the test and test data can be enabled. > So given serialization was already broken in the last release does it make any sense to fix it for 1.9.0 to 1.10.0 for the new field? According to the number of Maven central downloads versions 1.8 and 1.9.0 are the most popular of commons-csv. So I think there is merit in fixing serialization issues from 1.9.0 to 1.10.0 Besides the PR already offers the fix, why abandon it? Will PR #276 be part of version 1.10.0? Do we have a volunteer to review and merge it? regards, Markus ________________________________ From: Alex Herbert <alex.d.herb...@gmail.com> Sent: Monday, October 17, 2022 19:53 To: Commons Developers List <dev@commons.apache.org> Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1 On Mon, 17 Oct 2022 at 17:11, <sma...@outlook.de> wrote: > > Hello > > > This is the logic from the current builder: > > DuplicateHeaderMode mode = allowDuplicateHeaderNames ? > > DuplicateHeaderMode.ALLOW_ALL : DuplicateHeaderMode.ALLOW_EMPTY > > This is true only if Builder.setAllowDuplicateHeaderNames is actually called, > which is at the library user's discretion. I was only discussing the deserialization path and the logic that would be required in custom deserialization code to support backwards compatibility. However I do not think this library is supporting Serializable going forward. It is already known to be broken for CSVRecord and now also CSVFormat. > > Otherwise CSVFormat.Builder.duplicateHeaderMode remains null. > Also, the method Builder.setDuplicateHeaderMode(DuplicateHeaderMode) accepts > null. A non-null parameter should be enforced. > > Besides, shouldn't the above statement be: > allowDuplicateHeaderNames ? ALLOW_ALL : DISALLOW > ? > > duplicateHeaderMode should default to DISALLOW for backward compatibility. Duplicate header support was added in 1.7. Before that I do not know what happened without checking the old code. It may have thrown an exception or silently ignored it. I would have to track down the Jira ticket for when the feature was added. > > Found another small bug in setNullString where member quotedNullString was > inconsistently written (missing write in setQuote): > > this.quotedNullString = quoteCharacter + nullString + > quoteCharacter; > > All of the above in pull request > https://github.com/apache/commons-csv/pull/276 > > @Alex would you review my PR please? Sure. You have added test data for CSVFormat for 1.7 and 1.8 and these do not work (commented out). I take it this means serialization has been broken since the CSVFormat.delimiter was changed from char to String in 1.9.0. So given serialization was already broken in the last release does it make any sense to fix it for 1.9.0 to 1.10.0 for the new field? I agree that the quotedNullString is inconsistently handled. Regards, Alex --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
