On Wed, Oct 19, 2022 at 9:52 AM <sma...@outlook.de> wrote: > > > 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?
It's not abandoned, it will be for the next go around. This is a not a blocker. > > Will PR #276 be part of version 1.10.0? No. > Do we have a volunteer to review and merge it? Me, later. TY, Gary > > 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 > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
