You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Gary Gregory <ga...@gmail.com> on 2022/10/16 12:48:50 UTC
[VOTE] Release Apache Commons CSV 1.10.0 based on RC1
To: dev@commons.apache.org
We have fixed a few bugs and added some enhancements since Apache
Commons CSV 1.9.0 was released, so I would like to release Apache
Commons CSV 1.10.0.
Apache Commons CSV 1.10.0 RC1 is available for review here:
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1 (svn
revision 57404)
The Git tag commons-csv-1.10.0-RC1 commit for this RC is
1bd1fd8e6065da9d07b5a3a1723b059246b14001 which you can browse here:
https://gitbox.apache.org/repos/asf?p=commons-csv.git;a=commit;h=1bd1fd8e6065da9d07b5a3a1723b059246b14001
You may checkout this tag using:
git clone https://gitbox.apache.org/repos/asf/commons-csv.git
--branch commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
Maven artifacts are here:
https://repository.apache.org/content/repositories/orgapachecommons-1600/org/apache/commons/commons-csv/1.10.0/
These are the artifacts and their hashes:
#Release SHA-512s
#Sun Oct 16 08:32:39 EDT 2022
Apache\ Commons\
CSV-1.10.0.spdx.rdf.xml=c8cf7495637ce5282c72aa8e8ef4b93f206fbbdf21c7b337a34daa3b89af9816073d0185118cab4cdbed5104cc7f090e81efb614594079b02829c96829fe8aeb
commons-csv-1.10.0-bin.tar.gz=5b0ff3f5cc4aeff6c2aa2c11e92ea8b1b1fbbcd098ba79d97bcfd9af3ea08a9ebc922e5237bda8abcb238eb0d2fe7a5ef27534d0d796a1fc77691684a698f3d1
commons-csv-1.10.0-bin.zip=a52caa6ccda5b830c133c965fd6843d8f960e1ce3ef83decf5435022481746093cf8b4308c1fded21a168b751724b4a9f5e72f3eca34381828968bff89e7239b
commons-csv-1.10.0-bom.json=0e3e6652e520afb697461a3f6b33458dc8ef1dd157c2b1442664b8f12f7504226916e64960cea5cc92dd2b923d18047ed921be60eb7febfbeffc8ac3e3254fc0
commons-csv-1.10.0-bom.xml=91e494b94eea35ee7d85c8d8dbdc5029f981e0c5a8b1aae2b0f3cd77d1cea0fb0fc8c857d18d41ca055d12c4c4eb1adf70582f178a88751b836861c3286283db
commons-csv-1.10.0-javadoc.jar=719656f5399e0889af2e8b40f49458d0dec6e8ae85413f36caa2187235c68d9e42502bf07c66c0bcc7c5c3427bc0aac51f9897581842d303501bbff5ec73bdc0
commons-csv-1.10.0-sources.jar=c78c8fa112ef35d6d6c061b05e08816ce516a3acd56d720c59dbdaeb9d1202b16294233bd2aa2fac91cfa759456f8d7e08951876d85decdf0b8fb215a8e31754
commons-csv-1.10.0-src.tar.gz=a0d3a7aedab567aaa32a3dd1ceddeff412e686f3fd2b0805993db7e3dd4804b4d5d59e70f1500f23e8d0ba6531832e9f20d0c298bd443d9825724d5d1b1c87b3
commons-csv-1.10.0-src.zip=5cbdbcf367c9dd78e76228817592d70378b3cd8d919fdcd7df5c3eb2d7ebd8bd47fbb17376a631efb640be4115d42d3cc7aafa866bfefdfe073d5a4fcf625e0b
commons-csv-1.10.0-test-sources.jar=0d77c791934609b176457df5c60fdd5bfa3c6f392375adef81e5e7cd31de7fb25bd91b0bbe60a9ff46acb62efc2a33660adff24850370b7546a71374aa267f94
commons-csv-1.10.0-tests.jar=9a08e4d2ec865d9e1dafc01e4ee58804490b2d50f886bac47a5235581077b31a15dda5b3f8f2adb5a70be8794cca886126d980d6d3722eec2e10471fc24e1940
I have tested this with 'mvn' and 'mvn -V -Duser.name=$my_apache_id
-Prelease -Ptest-deploy -P jacoco -P japicmp clean package site
deploy' using:
Darwin *** 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:17:10
PDT 2022; root:xnu-8020.140.49~2/RELEASE_X86_64 x86_64
Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)
Maven home: /usr/local/Cellar/maven/3.8.6/libexec
Java version: 1.8.0_345, vendor: Homebrew, runtime:
/usr/local/Cellar/openjdk@8/1.8.0+345/libexec/openjdk.jdk/Contents/Home/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "12.6", arch: "x86_64", family: "mac"
Details of changes since 1.9.0 are in the release notes:
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/RELEASE-NOTES.txt
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/changes-report.html
Site:
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/index.html
(note some *relative* links are broken and the 1.10.0 directories
are not yet created - these will be OK once the site is deployed.)
JApiCmp Report (compared to 1.9.0):
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/japicmp.html
RAT Report:
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/rat-report.html
KEYS:
https://downloads.apache.org/commons/KEYS
Please review the release candidate and vote.
This vote will close no sooner than 72 hours from now.
[ ] +1 Release these artifacts
[ ] +0 OK, but...
[ ] -0 OK, but really should fix...
[ ] -1 I oppose this release because...
Thank you,
Gary Gregory,
Release Manager (using key 86fdc7e2a11262cb)
For following is intended as a helper and refresher for reviewers.
Validating a release candidate
==============================
These guidelines are NOT complete.
Requirements: Git, Java, Maven.
You can validate a release from a release candidate (RC) tag as follows.
1) Clone and checkout the RC tag
git clone https://gitbox.apache.org/repos/asf/commons-csv.git --branch
commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
cd commons-csv-1.10.0-RC1
2) Check Apache licenses
This step is not required if the site includes a RAT report page which
you then must check.
mvn apache-rat:check
3) Check binary compatibility
Use JApiCmp with the japicmp Maven Profile:
This step is not required if the site includes a JApiCmp report page
which you then must check.
mvn install -DskipTests -P japicmp japicmp:cmp
4) Build the package
mvn -V clean package
You can record the Maven and Java version produced by -V in your VOTE reply.
To gather OS information from a command line:
Windows: ver
Linux: uname -a
5) Build the site for a single module project
Note: Some plugins require the components to be installed instead of packaged.
mvn site
Check the site reports in:
- Windows: target\site\index.html
- Linux: target/site/index.html
-the end-
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[CANCEL][VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by "Gary D. Gregory" <gg...@apache.org>.
This VOTE is canceled. I will roll an RC2 if we agree that git master is OK.
Gary
On 2022/10/22 12:56:40 "Gary D. Gregory" wrote:
> Thank you for your excellent investigation, Alex, and for finding and fixing the missing Test annotations. Overall, we have 98% coverage.
>
> Taking stock, I think that this is where we are, we have main 3 issues:
>
> 1) Compatibility of duplicate header behavior from version to version
> 2) Validation bug with empty headers
> 3) Writing and parsing behavior mismatch
>
> One at a time:
>
> 1) Compatibility of duplicate header behavior from version to version
>
> - We have behavior [A] from 1.0 to 1.6.
> - We have behavior [B] from 1.7 to 1.9.
> - We are discussing if 1.10.0 should have behavior [A] or [B]
>
> I would vote for keeping behavior B for simplicity and keeping with the principle of least surprise: Maintaining the current behavior is better than flip-flopping.
>
> 2) Validation bug with empty headers
>
> Fixed in git master commit 289ffa16275c518722c6cda913bfca8a6e1a1411; please do verify the tests. I've added 3 test methods per Alex's failing examples below.
>
> 3) Writing and parsing behavior mismatch
>
> I've updated the documentation in git master as Alex suggested.
>
> I will cancel the RC1 vote in a separate message with a [CANCEL] subject prefix.
>
> Gary
>
> [A] From Alex:
> > From my investigation the behaviour from 1.0 to 1.6 was to throw on
> > duplicates, but ignore empty duplicates (so allowing pad columns in a
> > CSV).
>
> [B] From Alex:
> > The allowDuplicates flag was added in 1.7 to allow duplicates. DEFAULT
> > behaviour was true! So the behavioural compatibility was broken in
> > release 1.7. Note there is also an allowMissingColumnNames flag. Logic
> > was:
> >
> > - Store column headers as we go
> > - For each new column header, check in the existing headers
> > - If present:
> > -- If not empty then throw if allowDuplicateHeaderNames=true
> > -- If empty then throw if allowMissingColumnNames=false
> >
> > There is some logic failure here. It would allow the first missing
> > column name for example; only the second empty missing column name
> > would trigger an exception.
>
>
> On 2022/10/21 16:33:23 Alex Herbert wrote:
> > From my investigation the behaviour from 1.0 to 1.6 was to throw on
> > duplicates, but ignore empty duplicates (so allowing pad columns in a
> > CSV).
> >
> > The allowDuplicates flag was added in 1.7 to allow duplicates. DEFAULT
> > behaviour was true! So the behavioural compatibility was broken in
> > release 1.7. Note there is also an allowMissingColumnNames flag. Logic
> > was:
> >
> > - Store column headers as we go
> > - For each new column header, check in the existing headers
> > - If present:
> > -- If not empty then throw if allowDuplicateHeaderNames=true
> > -- If empty then throw if allowMissingColumnNames=false
> >
> > There is some logic failure here. It would allow the first missing
> > column name for example; only the second empty missing column name
> > would trigger an exception.
> >
> > The current master has updated this logic somewhat but I think the
> > concept is still the same. There are tests to show that the default
> > enum value is ALLOW_ALL. This is the same behaviour as 1.7, but not
> > 1.0 - 1.6.
> >
> > Setting the boolean flag to true -> ALLOW_ALL.
> > Setting the boolean flag to false -> ALLOW_EMPTY.
> >
> > So I think the default settings are correct, and the use of the old
> > flag flips the behaviour correctly. But when investigating I found
> > another issue.
> >
> > I looked at the CSVFormat tests and found two fixtures missing the
> > @Test annotation:
> >
> > testDuplicateHeaderElementsTrue()
> > testDuplicateHeaderElementsTrue_Deprecated()
> >
> > I added these to master.
> >
> > The tests for duplicate headers use the builder and setHeader("A",
> > "A"). So there are no tests for empty headers. Trying this throws an
> > exception in CSVFormat.validate for all of these:
> >
> > CSVFormat.DEFAULT.builder().setAllowDuplicateHeaderNames(false).setHeader("A",
> > "", "B", "").build();
> > CSVFormat.DEFAULT.builder().setDuplicateHeaderMode(DuplicateHeaderMode.ALLOW_EMPTY).setHeader("A",
> > "", "B", "").build();
> > CSVFormat.DEFAULT.builder().setAllowDuplicateHeaderNames(false)
> > .setAllowMissingColumnNames(true).setHeader("A", "", "B", "").build();
> >
> > So it seems the logic when building a CSVFormat with explicit headers
> > is different from when parsing a format in CSVParser.createHeaders().
> > That method has a lot of logic to set up the header map. If a
> > CSVFormat is created with an explicit header the behaviour is
> > different with respect to duplicate headers, and
> > allowMissingColumnNames which is not even considered in
> > CSVFormat.validate().
> >
> > This is the current behaviour in master for the parser:
> >
> > @Test
> > public void testEmptyColumnsAndDuplicateHeadersNotAllowed() throws Exception {
> > assertThrows(IllegalArgumentException.class,
> > () -> CSVParser.parse("a,,c,\n1,2,3,4\nw,x,y,z",
> > CSVFormat.DEFAULT.withHeader().withAllowDuplicateHeaderNames(false)));
> > }
> >
> > @Test
> > public void testEmptyColumnsAndDuplicateHeadersNotAllowedWithMissingColumnsAllowed()
> > throws Exception {
> > try (CSVParser parser = CSVParser.parse("a,,c,\n1,2,3,4\nw,x,y,z",
> > CSVFormat.DEFAULT.withHeader().withAllowDuplicateHeaderNames(false).withAllowMissingColumnNames(true)))
> > {
> > // noop
> > }
> > }
> >
> > So the duplicate headers cannot be considered on its own, it requires
> > use of the allow missing column names flag.
> >
> > Also, if a CSVFormat has an explicit header then the CSVParser will
> > not construct a header Map<String, Integer> to allow column names to
> > look up column indices.
> >
> > I am not sure of the best path to fix this. I would document the
> > CSVParser header map as not being created if the header is not read
> > from the CSV input (current behaviour). Then maybe change the logic in
> > CSVFormat.validate() to respect the ALLOW_ALL, ALLOW_EMPTY, ALLOW_NONE
> > duplicates choice, possibly also considering allowEmptyColumnNames
> > which defaults to false. Ideally we should get the same exceptions
> > when building a CSVFormat with explicitly set headers, and parsing a
> > CSV file and getting the header from the file. Currently I do not
> > think that is true.
> >
> > Alex
> >
> > On Fri, 21 Oct 2022 at 15:54, Gary Gregory <ga...@gmail.com> wrote:
> > >
> > > Ok, I don't see any conflict anymore but the PR title does not match the
> > > code: "CSVFormat.duplicateHeaderMode requires default DISALLOW"
> > >
> > > I am ok with the changes as they stand now but we have to resolve if the
> > > default should be changed.
> > >
> > > Gary
> > >
> > > On Fri, Oct 21, 2022, 09:44 <sm...@outlook.de> wrote:
> > >
> > > > Hi Gary,
> > > >
> > > > the PR did not have conflicts 30 minutes ago. So we must have encountered
> > > > a race condition between you updating master and me rebasing and
> > > > force-pushing my pr.
> > > > I've done that again just now. There are no conflicts.
> > > > At the same time I see parts of my PR have dribbled into master already,
> > > > so if you rather implement the changes yourself, please go ahead. I am fine
> > > > with closing my pr.
> > > >
> > > > regards,
> > > > Markus
> > > >
> > > >
> > > > From: Gary Gregory <ga...@gmail.com>
> > > > Sent: Friday, October 21, 2022 15:17
> > > > To: Commons Developers List <de...@commons.apache.org>
> > > > Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
> > > >
> > > > Hi Markus,
> > > >
> > > > The PR has conflicts and does not test its changes.
> > > >
> > > > I'd like feedback from the community on whether or not git master as it is
> > > > now is OK for the duplicate header behavior or if changing the default is
> > > > needed and if doing so is just ping-pongonging from one incompatibility to
> > > > another, based on previous versions.
> > > >
> > > > TY!
> > > >
> > > > Gary
> > > >
> > > > On Fri, Oct 21, 2022, 08:52 <sm...@outlook.de> wrote:
> > > >
> > > > > I've removed serialization stuff from the PR and rebased it to master
> > > > > https://github.com/apache/commons-csv/pull/276
> > > > >
> > > > > kind regards,
> > > > > Markus
> > > > >
> > > > > From: Gary D. Gregory <gg...@apache.org>
> > > > > Sent: Friday, October 21, 2022 14:18
> > > > > To: dev@commons.apache.org <de...@commons.apache.org>
> > > > > Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
> > > > >
> > > > > On 2022/10/20 22:56:05 Alex Herbert wrote:
> > > > > > On Thu, 20 Oct 2022 at 23:43, Alex Herbert <al...@gmail.com>
> > > > > wrote:
> > > > > > >
> > > > > > > I did not have time to track through whether this behaviour changed
> > > > > > > after the initial implementation of the flag. I would think not as
> > > > the
> > > > > > > original behaviour is from 1.0. This would map to:
> > > > > > >
> > > > > > > true -> ALLOW_ALL
> > > > > > > false -> ALLOW_EMPTY
> > > > > > > new -> DISALLOW
> > > > > > >
> > > > > > > Which is what we currently have in 1.10.0 RC1. Thus the PR #276 [7]
> > > > to
> > > > > > > change the use of the flag to 'false -> DISALLOW' is not maintaining
> > > > > > > behavioural compatibility (to 1.7, or back to 1.0).
> > > > > >
> > > > > > PS. I just verified that PR 276 changes the DuplicateHeaderMode value
> > > > > > for allowDuplicates=false and does not change any tests.
> > > > > >
> > > > > > So the test suite is currently not enforcing behavioural
> > > > > > compatibility. This seems like a glaring hole in the tests and should
> > > > > > be addressed to prevent regressions.
> > > > >
> > > > > In git master, there are many tests for senders of
> > > > > withAllowDuplicateHeaderNames(boolean) and
> > > > > setAllowDuplicateHeaderNames(boolean).
> > > > >
> > > > > I filled in the coverage in testGetAllowDuplicateHeaderNames() for
> > > > > getAllowDuplicateHeaderNames() to reflect the _current_ behavior, right
> > > > or
> > > > > wrong.
> > > > >
> > > > > Let's reassess now git master now please. If the PR matters still, it
> > > > > should be rebased on git master.
> > > > >
> > > > > Gary
> > > > >
> > > > > >
> > > > > > ---------------------------------------------------------------------
> > > > > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > > > > For additional commands, e-mail: dev-help@commons.apache.org
> > > > > >
> > > > > >
> > > > >
> > > > > ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > > > For additional commands, e-mail: dev-help@commons.apache.org
> > > > >
> > > > > ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > > > For additional commands, e-mail: dev-help@commons.apache.org
> > > > >
> > > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > > For additional commands, e-mail: dev-help@commons.apache.org
> > > >
> > > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [csv] validation of duplicate headers (was [VOTE] Release Apache Commons CSV 1.10.0 based on RC1)
Posted by Alex Herbert <al...@gmail.com>.
On Tue, 25 Oct 2022 at 17:29, Alex Herbert <al...@gmail.com> wrote:
>
> On Sun, 23 Oct 2022 at 19:44, Alex Herbert <al...@gmail.com> wrote:
> >
> > Summary:
> >
> > 1. Should CSVParser treat null and blank headers as the same when
> > checking for duplicates, i.e. all are considered an 'empty' name? This
> > is current CSVFormat behaviour.
> > 2. Should CSVFormat respect ignoreHeaderCase when checking for
> > duplicates? This is current CSVParser behaviour.
> > 3. Should blank column names be sanitised to the empty string ""? This
> > is not current behaviour but is the logical behaviour for checking
> > duplicates in CSVFormat.
>
> I have proposed a fix for this in PR #279 [1]. It maintains a flag
> that notes when any type of missing header name has occurred, Thus it
> now throws when a duplicate null is found when using
> DuplicateHeaderMode.DISALLOW.
>
> I marked the PR as a WIP. It should probably have an associated Jira
> ticket to track this change if merged. Or it could be added to CSV-264
> as further details of that fix [2].
>
> I have not updated the documentation for ignoreHeaderCase to address item 2.
>
> The functionality with regard to the header map is unchanged since the
> header map does not store null headers, and any missing headers are
> not modified (i.e. they are not all sanitised to the empty string "").
>
> Alex
>
> [1] https://github.com/apache/commons-csv/pull/279
> [2] https://issues.apache.org/jira/browse/CSV-264
PR now updated with:
- Documentation of the parser specific flag 'ignore header case'
- CSVDuplicateHeaderTest to have test cases using the case insensitive
duplicates
I believe this to be all that is required to fix the issues with
handling duplicate header names.
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [csv] validation of duplicate headers (was [VOTE] Release Apache Commons CSV 1.10.0 based on RC1)
Posted by Alex Herbert <al...@gmail.com>.
On Sun, 23 Oct 2022 at 19:44, Alex Herbert <al...@gmail.com> wrote:
>
> Summary:
>
> 1. Should CSVParser treat null and blank headers as the same when
> checking for duplicates, i.e. all are considered an 'empty' name? This
> is current CSVFormat behaviour.
> 2. Should CSVFormat respect ignoreHeaderCase when checking for
> duplicates? This is current CSVParser behaviour.
> 3. Should blank column names be sanitised to the empty string ""? This
> is not current behaviour but is the logical behaviour for checking
> duplicates in CSVFormat.
I have proposed a fix for this in PR #279 [1]. It maintains a flag
that notes when any type of missing header name has occurred, Thus it
now throws when a duplicate null is found when using
DuplicateHeaderMode.DISALLOW.
I marked the PR as a WIP. It should probably have an associated Jira
ticket to track this change if merged. Or it could be added to CSV-264
as further details of that fix [2].
I have not updated the documentation for ignoreHeaderCase to address item 2.
The functionality with regard to the header map is unchanged since the
header map does not store null headers, and any missing headers are
not modified (i.e. they are not all sanitised to the empty string "").
Alex
[1] https://github.com/apache/commons-csv/pull/279
[2] https://issues.apache.org/jira/browse/CSV-264
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[csv] validation of duplicate headers (was [VOTE] Release Apache Commons CSV 1.10.0 based on RC1)
Posted by Alex Herbert <al...@gmail.com>.
On Sun, 23 Oct 2022 at 14:09, Gary D. Gregory <gg...@apache.org> wrote:
>
> Hi All, Alex, more below:
>
> On 2022/10/22 21:23:13 Alex Herbert wrote:
> > On Sat, 22 Oct 2022 at 20:05, Gary D. Gregory <gg...@apache.org> wrote:
> > >
> > > Thank you for the new tests Alex!
> > >
> > > Here is one area that is easy to overlook: As Commons CSV has evolved, _not all settings in CSVFormat_ apply to both writing and parsing. To wit, the existing Javadoc for CSVFormat.getAllowMissingColumnNames(): "Gets whether missing column names are allowed when parsing the header line."
> > >
> > > I've updated the Javadoc for CSVFormat.Builder.setAllowMissingColumnNames(boolean) to state this is a parser setting.
> > >
> > > I've also updated the test since the commented-out test data is valid for parsing.
> > >
> > > In git master now.
> >
> > OK, thanks for the explanation. If the CSVFormat ignores the
> > allowEmptyColumnsNames in the validate() method it should act as if
> > the setting is true, i.e. only the DuplicateHeaderMode should be used
> > to validate the header.
>
> That's how it is now, allowEmptyColumnsNames cannot be considered in the validate method because a CSVFormat does not know if it is going to be used for parsing or writing.
>
> So the test can be updated to stream the full
> > set of cases (reinstating those you commented out) and filter to those
> > where the flag is set to true, e.g. using:
>
> I already had added a new test data provider method to do that so, all I had to do now is make sure that when I deleted the commented out test elements, these were covered in the 2nd data provider called duplicateHeaderParseOnlyData().
>
> >
> > ---
> >
> > static Stream<Arguments> duplicateHeaderAllowsMissingColumnsNamesData() {
> > return duplicateHeaderData().filter(arg ->
> > Boolean.TRUE.equals(arg.get()[1]));
> > }
>
> I did not do this because it seemed (to me) clearer (less magical) to create a duplicateHeaderParseOnlyData() and list explicitly test cases there. WDYT?
I prefer to keep all the parser test data in one method. For the
CSVFormat data, it can be reused and filtered based on the
allowMissingColumnNames flag. The CSVFormat 'should' behave like the
CSVParser when allowMissingColumnNames is true. It makes sense for the
CSVFormat test cases to be a subset of the CSVParser. Note this may
not always be the case. However with the current code this is mostly
true (see below) and it simplifies adding all the test cases.
I added some more test cases and have found a bug and some more issues.
Bugs
This errors:
CSVFormat:
Arguments.of(DuplicateHeaderMode.DISALLOW, true, new String[]
{"A", ""}, true),
There is no duplicate, so nothing should be triggered. But it triggers
on the first empty header. I have fixed this in git master to sanitise
any blank as the empty string "" before checking for duplicates. Thus
only a second duplicate empty name will throw. However the names are
only sanitised for validate(). The original names are stored as is,
thus the CSVFormat.getHeader() will return the original blank names,
for example using two different blanks as {" ", " "}.
Issues:
In CSVParser using the isBlank method labels an empty header. But
headers are added to the map raw (without trim()). Thus this errors:
CSVParser:
Arguments.of(DuplicateHeaderMode.DISALLOW, true, new String[] {"
", " "}, false),
When the blank strings are different lengths they are both identified
as empty. However they are checked in the duplicates map using
different strings, thus no duplicate is identified, and no exception
raised for a duplicate blank header. Thus at present the CSVFormat and
CSVParser have a different interpretation of what is a duplicate empty
header.
I think that CSVParser should be updated to sanitise all non-null
blank strings to the empty string with regard to duplicates. But this
has to be handled differently to the CSVFormat which keeps a throw
away Set<String> of headers. The CSVParser builds a map using the
current headers. To avoid changing the blank headers to "" would
require keeping a flag to mark an observation of a blank header. The
existing header map functionality can then be left unchanged. This
allows the headers from getHeaders() to contain different blank
strings after parsing.
At present the CSVFormat treats null names and blank names as empty.
So you cannot have an input of {"", null} with DISALLOW. But this is
allowed for CSVParser. That currently ignores null names from
duplicate checking. This is different behaviour from CSVFormat.
I have not made any more changes as I wanted to have a second opinion.
Presently all the test cases in the CSVDuplicateHeaderTest pass for
CSVFormat when allowEmptyColumnNames=true. The commented out cases
have a different result in CSVParser as it ignores duplicates of null
and blank.
I believe the CSVParser handling of null headers is simply because it
uses a Map<String, Integer> to map column names to a column index.
This will not work for null keys when the implementation is a TreeMap
(one choice for the map). So null keys are ignored from the duplicate
check. This leads to one further complication: the CSVParser respects
the CSVFormat.getIgnoreHeaderCase() and uses a TreeMap with an
ignoreCase comparator, otherwise a LinkedHashMap, to store the header
names. So {"A", "a"} can be duplicates.
I have not yet added the ignoreHeaderCase support to
CSVDuplicateHeaderTest. This would fail for CSVFormat. It can be
easily fixed by using a TreeSet with the ignoreCase comparator or else
a HashSet. But I do not support doing this as the ignoreHeaderCase
setting is only for parsing.
Summary:
1. Should CSVParser treat null and blank headers as the same when
checking for duplicates, i.e. all are considered an 'empty' name? This
is current CSVFormat behaviour.
2. Should CSVFormat respect ignoreHeaderCase when checking for
duplicates? This is current CSVParser behaviour.
3. Should blank column names be sanitised to the empty string ""? This
is not current behaviour but is the logical behaviour for checking
duplicates in CSVFormat.
IMO I think we can fix 1. Note that null, "" and " " throws if
allowEmptyColumnNames is false in the parser. Thus the parser
currently considers all these as the same (empty) for that setting.
The implementation of duplicate checking is just not matching this,
and I believe this is due to using a TreeMap under some conditions
which cannot handle null.
For 2 it seems that ignoreHeaderCase is a setting for parsing. It
affects use of the header names, specifically the header map from the
parser. It does not apply to writing. So this may just require some
extra documentation in the CSVFormat properties and updates to
CSVDuplicateHeaderTest to appropriately test it. It would mean that
checking for duplicate header names can be stricter in the parser than
CSVFormat.
We leave 3 which allows the headers to pass through unchanged and they
can be retrieved using CSVFormat.getHeader() or
CSVParser.getHeaderNames(). The maintains backwards behavioural
compatibility. Note that this allows the parser to be created to allow
duplicate empty names and the resulting map has entries for different
empty name keys such as "" and " ". I would consider this an artifact
where these empty names should not be used by the caller who is
interested in the non-empty column names.
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Gary Gregory <ga...@gmail.com>.
On Mon, Oct 24, 2022 at 4:19 PM Alex Herbert <al...@gmail.com> wrote:
>
> On Sun, 23 Oct 2022 at 14:09, Gary D. Gregory <gg...@apache.org> wrote:
> >
> > Ah, well, let's have you review git master now and feel free to refactor. I think we are close if not done for another RC. WDYT?
>
> Since this thread was for the RC1 vote I started a new thread titled:
>
> [csv] validation of duplicate headers (was [VOTE] Release Apache
> Commons CSV 1.10.0 based on RC1)
>
> I found one bug in the implementation is master which I fixed for
> CSVFormat. There is still some inconsistent behaviour by CSVParser
> that I discuss in the new thread that should be fixed, or current
> behaviour documented, before an RC2.
Thanks Alex for putting in the time and effort, needs careful consideration...
Gary
>
> Alex
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Alex Herbert <al...@gmail.com>.
On Sun, 23 Oct 2022 at 14:09, Gary D. Gregory <gg...@apache.org> wrote:
>
> Ah, well, let's have you review git master now and feel free to refactor. I think we are close if not done for another RC. WDYT?
Since this thread was for the RC1 vote I started a new thread titled:
[csv] validation of duplicate headers (was [VOTE] Release Apache
Commons CSV 1.10.0 based on RC1)
I found one bug in the implementation is master which I fixed for
CSVFormat. There is still some inconsistent behaviour by CSVParser
that I discuss in the new thread that should be fixed, or current
behaviour documented, before an RC2.
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by "Gary D. Gregory" <gg...@apache.org>.
Hi All, Alex, more below:
On 2022/10/22 21:23:13 Alex Herbert wrote:
> On Sat, 22 Oct 2022 at 20:05, Gary D. Gregory <gg...@apache.org> wrote:
> >
> > Thank you for the new tests Alex!
> >
> > Here is one area that is easy to overlook: As Commons CSV has evolved, _not all settings in CSVFormat_ apply to both writing and parsing. To wit, the existing Javadoc for CSVFormat.getAllowMissingColumnNames(): "Gets whether missing column names are allowed when parsing the header line."
> >
> > I've updated the Javadoc for CSVFormat.Builder.setAllowMissingColumnNames(boolean) to state this is a parser setting.
> >
> > I've also updated the test since the commented-out test data is valid for parsing.
> >
> > In git master now.
>
> OK, thanks for the explanation. If the CSVFormat ignores the
> allowEmptyColumnsNames in the validate() method it should act as if
> the setting is true, i.e. only the DuplicateHeaderMode should be used
> to validate the header.
That's how it is now, allowEmptyColumnsNames cannot be considered in the validate method because a CSVFormat does not know if it is going to be used for parsing or writing.
So the test can be updated to stream the full
> set of cases (reinstating those you commented out) and filter to those
> where the flag is set to true, e.g. using:
I already had added a new test data provider method to do that so, all I had to do now is make sure that when I deleted the commented out test elements, these were covered in the 2nd data provider called duplicateHeaderParseOnlyData().
>
> ---
>
> static Stream<Arguments> duplicateHeaderAllowsMissingColumnsNamesData() {
> return duplicateHeaderData().filter(arg ->
> Boolean.TRUE.equals(arg.get()[1]));
> }
I did not do this because it seemed (to me) clearer (less magical) to create a duplicateHeaderParseOnlyData() and list explicitly test cases there. WDYT?
>
> @ParameterizedTest
> @MethodSource(value = {"duplicateHeaderAllowsMissingColumnsNamesData"})
> public void testCSVFormat(final DuplicateHeaderMode duplicateHeaderMode, ...
>
> ---
>
> This finds the following failure:
>
> Arguments.of(DuplicateHeaderMode.ALLOW_EMPTY, true, new String[] {"
> ", " "}, true),
>
> This is due to the difference in the CSVParser and CSVFormat in the
> use of the trim() method to detect an empty header:
>
> CSVParser:
> final boolean emptyHeader = header == null || header.trim().isEmpty();
>
> CSVFormat:
> final boolean empty = header == null || header.isEmpty();
>
> This means that the definition of 'empty' is different for the
> CSVFormat and the CSVParser. Should this be corrected in CSVFormat to
> use trim()?
I refactored the logic in CSVFormat#isBlank(String) where the "blank" concept vs. "empty" is the same as in Apache Commons Lang where "empty" means size 0 and "blank" means "trims to size 0"; see org.apache.commons.lang3.StringUtils.isBlank(CharSequence).
>
> If so then I can push the updated test and fix to master. That would
> remove the requirement to comment out any of the test cases and moving
> those to a different test stream method.
Ah, well, let's have you review git master now and feel free to refactor. I think we are close if not done for another RC. WDYT?
TY!
Gary
>
> Alex
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Alex Herbert <al...@gmail.com>.
On Sat, 22 Oct 2022 at 20:05, Gary D. Gregory <gg...@apache.org> wrote:
>
> Thank you for the new tests Alex!
>
> Here is one area that is easy to overlook: As Commons CSV has evolved, _not all settings in CSVFormat_ apply to both writing and parsing. To wit, the existing Javadoc for CSVFormat.getAllowMissingColumnNames(): "Gets whether missing column names are allowed when parsing the header line."
>
> I've updated the Javadoc for CSVFormat.Builder.setAllowMissingColumnNames(boolean) to state this is a parser setting.
>
> I've also updated the test since the commented-out test data is valid for parsing.
>
> In git master now.
OK, thanks for the explanation. If the CSVFormat ignores the
allowEmptyColumnsNames in the validate() method it should act as if
the setting is true, i.e. only the DuplicateHeaderMode should be used
to validate the header. So the test can be updated to stream the full
set of cases (reinstating those you commented out) and filter to those
where the flag is set to true, e.g. using:
---
static Stream<Arguments> duplicateHeaderAllowsMissingColumnsNamesData() {
return duplicateHeaderData().filter(arg ->
Boolean.TRUE.equals(arg.get()[1]));
}
@ParameterizedTest
@MethodSource(value = {"duplicateHeaderAllowsMissingColumnsNamesData"})
public void testCSVFormat(final DuplicateHeaderMode duplicateHeaderMode, ...
---
This finds the following failure:
Arguments.of(DuplicateHeaderMode.ALLOW_EMPTY, true, new String[] {"
", " "}, true),
This is due to the difference in the CSVParser and CSVFormat in the
use of the trim() method to detect an empty header:
CSVParser:
final boolean emptyHeader = header == null || header.trim().isEmpty();
CSVFormat:
final boolean empty = header == null || header.isEmpty();
This means that the definition of 'empty' is different for the
CSVFormat and the CSVParser. Should this be corrected in CSVFormat to
use trim()?
If so then I can push the updated test and fix to master. That would
remove the requirement to comment out any of the test cases and moving
those to a different test stream method.
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by "Gary D. Gregory" <gg...@apache.org>.
Thank you for the new tests Alex!
Here is one area that is easy to overlook: As Commons CSV has evolved, _not all settings in CSVFormat_ apply to both writing and parsing. To wit, the existing Javadoc for CSVFormat.getAllowMissingColumnNames(): "Gets whether missing column names are allowed when parsing the header line."
I've updated the Javadoc for CSVFormat.Builder.setAllowMissingColumnNames(boolean) to state this is a parser setting.
I've also updated the test since the commented-out test data is valid for parsing.
In git master now.
Gary
On 2022/10/22 17:37:15 Alex Herbert wrote:
> On Sat, 22 Oct 2022 at 13:56, Gary D. Gregory <gg...@apache.org> wrote:
> >
> > Thank you for your excellent investigation, Alex, and for finding and fixing the missing Test annotations. Overall, we have 98% coverage.
>
> :)
>
> > Taking stock, I think that this is where we are, we have main 3 issues:
> >
> > 1) Compatibility of duplicate header behavior from version to version
> > 2) Validation bug with empty headers
> > 3) Writing and parsing behavior mismatch
> >
> > One at a time:
> >
> > 1) Compatibility of duplicate header behavior from version to version
> >
> > - We have behavior [A] from 1.0 to 1.6.
> > - We have behavior [B] from 1.7 to 1.9.
> > - We are discussing if 1.10.0 should have behavior [A] or [B]
> >
> > I would vote for keeping behavior B for simplicity and keeping with the principle of least surprise: Maintaining the current behavior is better than flip-flopping.
>
> +1
>
> > 2) Validation bug with empty headers
> >
> > Fixed in git master commit 289ffa16275c518722c6cda913bfca8a6e1a1411; please do verify the tests. I've added 3 test methods per Alex's failing examples below.
>
> The fix does not address the allowMissingColumnNames flag. This flag
> is not mentioned in the Jira tickets for duplicate column names but it
> is used by the CSVParser and changes behaviour.
>
> I added a new test class that has all the combinations of
> DuplicateHeaderMode and allowMissingColumnNames with various headers.
> The test has 36 cases and all are passed by the CSVParser. 6 are
> currently not passed by CSVFormat and so have been disabled. Please
> review git master commit 24ffa7b4cf9cc392c4fd6a70363f103d40d1d1a7. Add
> more cases for other headers if required.
>
> > 3) Writing and parsing behavior mismatch
> >
> > I've updated the documentation in git master as Alex suggested.
>
> Thanks.
>
> Alex
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Alex Herbert <al...@gmail.com>.
On Sat, 22 Oct 2022 at 13:56, Gary D. Gregory <gg...@apache.org> wrote:
>
> Thank you for your excellent investigation, Alex, and for finding and fixing the missing Test annotations. Overall, we have 98% coverage.
:)
> Taking stock, I think that this is where we are, we have main 3 issues:
>
> 1) Compatibility of duplicate header behavior from version to version
> 2) Validation bug with empty headers
> 3) Writing and parsing behavior mismatch
>
> One at a time:
>
> 1) Compatibility of duplicate header behavior from version to version
>
> - We have behavior [A] from 1.0 to 1.6.
> - We have behavior [B] from 1.7 to 1.9.
> - We are discussing if 1.10.0 should have behavior [A] or [B]
>
> I would vote for keeping behavior B for simplicity and keeping with the principle of least surprise: Maintaining the current behavior is better than flip-flopping.
+1
> 2) Validation bug with empty headers
>
> Fixed in git master commit 289ffa16275c518722c6cda913bfca8a6e1a1411; please do verify the tests. I've added 3 test methods per Alex's failing examples below.
The fix does not address the allowMissingColumnNames flag. This flag
is not mentioned in the Jira tickets for duplicate column names but it
is used by the CSVParser and changes behaviour.
I added a new test class that has all the combinations of
DuplicateHeaderMode and allowMissingColumnNames with various headers.
The test has 36 cases and all are passed by the CSVParser. 6 are
currently not passed by CSVFormat and so have been disabled. Please
review git master commit 24ffa7b4cf9cc392c4fd6a70363f103d40d1d1a7. Add
more cases for other headers if required.
> 3) Writing and parsing behavior mismatch
>
> I've updated the documentation in git master as Alex suggested.
Thanks.
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by "Gary D. Gregory" <gg...@apache.org>.
Thank you for your excellent investigation, Alex, and for finding and fixing the missing Test annotations. Overall, we have 98% coverage.
Taking stock, I think that this is where we are, we have main 3 issues:
1) Compatibility of duplicate header behavior from version to version
2) Validation bug with empty headers
3) Writing and parsing behavior mismatch
One at a time:
1) Compatibility of duplicate header behavior from version to version
- We have behavior [A] from 1.0 to 1.6.
- We have behavior [B] from 1.7 to 1.9.
- We are discussing if 1.10.0 should have behavior [A] or [B]
I would vote for keeping behavior B for simplicity and keeping with the principle of least surprise: Maintaining the current behavior is better than flip-flopping.
2) Validation bug with empty headers
Fixed in git master commit 289ffa16275c518722c6cda913bfca8a6e1a1411; please do verify the tests. I've added 3 test methods per Alex's failing examples below.
3) Writing and parsing behavior mismatch
I've updated the documentation in git master as Alex suggested.
I will cancel the RC1 vote in a separate message with a [CANCEL] subject prefix.
Gary
[A] From Alex:
> From my investigation the behaviour from 1.0 to 1.6 was to throw on
> duplicates, but ignore empty duplicates (so allowing pad columns in a
> CSV).
[B] From Alex:
> The allowDuplicates flag was added in 1.7 to allow duplicates. DEFAULT
> behaviour was true! So the behavioural compatibility was broken in
> release 1.7. Note there is also an allowMissingColumnNames flag. Logic
> was:
>
> - Store column headers as we go
> - For each new column header, check in the existing headers
> - If present:
> -- If not empty then throw if allowDuplicateHeaderNames=true
> -- If empty then throw if allowMissingColumnNames=false
>
> There is some logic failure here. It would allow the first missing
> column name for example; only the second empty missing column name
> would trigger an exception.
On 2022/10/21 16:33:23 Alex Herbert wrote:
> From my investigation the behaviour from 1.0 to 1.6 was to throw on
> duplicates, but ignore empty duplicates (so allowing pad columns in a
> CSV).
>
> The allowDuplicates flag was added in 1.7 to allow duplicates. DEFAULT
> behaviour was true! So the behavioural compatibility was broken in
> release 1.7. Note there is also an allowMissingColumnNames flag. Logic
> was:
>
> - Store column headers as we go
> - For each new column header, check in the existing headers
> - If present:
> -- If not empty then throw if allowDuplicateHeaderNames=true
> -- If empty then throw if allowMissingColumnNames=false
>
> There is some logic failure here. It would allow the first missing
> column name for example; only the second empty missing column name
> would trigger an exception.
>
> The current master has updated this logic somewhat but I think the
> concept is still the same. There are tests to show that the default
> enum value is ALLOW_ALL. This is the same behaviour as 1.7, but not
> 1.0 - 1.6.
>
> Setting the boolean flag to true -> ALLOW_ALL.
> Setting the boolean flag to false -> ALLOW_EMPTY.
>
> So I think the default settings are correct, and the use of the old
> flag flips the behaviour correctly. But when investigating I found
> another issue.
>
> I looked at the CSVFormat tests and found two fixtures missing the
> @Test annotation:
>
> testDuplicateHeaderElementsTrue()
> testDuplicateHeaderElementsTrue_Deprecated()
>
> I added these to master.
>
> The tests for duplicate headers use the builder and setHeader("A",
> "A"). So there are no tests for empty headers. Trying this throws an
> exception in CSVFormat.validate for all of these:
>
> CSVFormat.DEFAULT.builder().setAllowDuplicateHeaderNames(false).setHeader("A",
> "", "B", "").build();
> CSVFormat.DEFAULT.builder().setDuplicateHeaderMode(DuplicateHeaderMode.ALLOW_EMPTY).setHeader("A",
> "", "B", "").build();
> CSVFormat.DEFAULT.builder().setAllowDuplicateHeaderNames(false)
> .setAllowMissingColumnNames(true).setHeader("A", "", "B", "").build();
>
> So it seems the logic when building a CSVFormat with explicit headers
> is different from when parsing a format in CSVParser.createHeaders().
> That method has a lot of logic to set up the header map. If a
> CSVFormat is created with an explicit header the behaviour is
> different with respect to duplicate headers, and
> allowMissingColumnNames which is not even considered in
> CSVFormat.validate().
>
> This is the current behaviour in master for the parser:
>
> @Test
> public void testEmptyColumnsAndDuplicateHeadersNotAllowed() throws Exception {
> assertThrows(IllegalArgumentException.class,
> () -> CSVParser.parse("a,,c,\n1,2,3,4\nw,x,y,z",
> CSVFormat.DEFAULT.withHeader().withAllowDuplicateHeaderNames(false)));
> }
>
> @Test
> public void testEmptyColumnsAndDuplicateHeadersNotAllowedWithMissingColumnsAllowed()
> throws Exception {
> try (CSVParser parser = CSVParser.parse("a,,c,\n1,2,3,4\nw,x,y,z",
> CSVFormat.DEFAULT.withHeader().withAllowDuplicateHeaderNames(false).withAllowMissingColumnNames(true)))
> {
> // noop
> }
> }
>
> So the duplicate headers cannot be considered on its own, it requires
> use of the allow missing column names flag.
>
> Also, if a CSVFormat has an explicit header then the CSVParser will
> not construct a header Map<String, Integer> to allow column names to
> look up column indices.
>
> I am not sure of the best path to fix this. I would document the
> CSVParser header map as not being created if the header is not read
> from the CSV input (current behaviour). Then maybe change the logic in
> CSVFormat.validate() to respect the ALLOW_ALL, ALLOW_EMPTY, ALLOW_NONE
> duplicates choice, possibly also considering allowEmptyColumnNames
> which defaults to false. Ideally we should get the same exceptions
> when building a CSVFormat with explicitly set headers, and parsing a
> CSV file and getting the header from the file. Currently I do not
> think that is true.
>
> Alex
>
> On Fri, 21 Oct 2022 at 15:54, Gary Gregory <ga...@gmail.com> wrote:
> >
> > Ok, I don't see any conflict anymore but the PR title does not match the
> > code: "CSVFormat.duplicateHeaderMode requires default DISALLOW"
> >
> > I am ok with the changes as they stand now but we have to resolve if the
> > default should be changed.
> >
> > Gary
> >
> > On Fri, Oct 21, 2022, 09:44 <sm...@outlook.de> wrote:
> >
> > > Hi Gary,
> > >
> > > the PR did not have conflicts 30 minutes ago. So we must have encountered
> > > a race condition between you updating master and me rebasing and
> > > force-pushing my pr.
> > > I've done that again just now. There are no conflicts.
> > > At the same time I see parts of my PR have dribbled into master already,
> > > so if you rather implement the changes yourself, please go ahead. I am fine
> > > with closing my pr.
> > >
> > > regards,
> > > Markus
> > >
> > >
> > > From: Gary Gregory <ga...@gmail.com>
> > > Sent: Friday, October 21, 2022 15:17
> > > To: Commons Developers List <de...@commons.apache.org>
> > > Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
> > >
> > > Hi Markus,
> > >
> > > The PR has conflicts and does not test its changes.
> > >
> > > I'd like feedback from the community on whether or not git master as it is
> > > now is OK for the duplicate header behavior or if changing the default is
> > > needed and if doing so is just ping-pongonging from one incompatibility to
> > > another, based on previous versions.
> > >
> > > TY!
> > >
> > > Gary
> > >
> > > On Fri, Oct 21, 2022, 08:52 <sm...@outlook.de> wrote:
> > >
> > > > I've removed serialization stuff from the PR and rebased it to master
> > > > https://github.com/apache/commons-csv/pull/276
> > > >
> > > > kind regards,
> > > > Markus
> > > >
> > > > From: Gary D. Gregory <gg...@apache.org>
> > > > Sent: Friday, October 21, 2022 14:18
> > > > To: dev@commons.apache.org <de...@commons.apache.org>
> > > > Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
> > > >
> > > > On 2022/10/20 22:56:05 Alex Herbert wrote:
> > > > > On Thu, 20 Oct 2022 at 23:43, Alex Herbert <al...@gmail.com>
> > > > wrote:
> > > > > >
> > > > > > I did not have time to track through whether this behaviour changed
> > > > > > after the initial implementation of the flag. I would think not as
> > > the
> > > > > > original behaviour is from 1.0. This would map to:
> > > > > >
> > > > > > true -> ALLOW_ALL
> > > > > > false -> ALLOW_EMPTY
> > > > > > new -> DISALLOW
> > > > > >
> > > > > > Which is what we currently have in 1.10.0 RC1. Thus the PR #276 [7]
> > > to
> > > > > > change the use of the flag to 'false -> DISALLOW' is not maintaining
> > > > > > behavioural compatibility (to 1.7, or back to 1.0).
> > > > >
> > > > > PS. I just verified that PR 276 changes the DuplicateHeaderMode value
> > > > > for allowDuplicates=false and does not change any tests.
> > > > >
> > > > > So the test suite is currently not enforcing behavioural
> > > > > compatibility. This seems like a glaring hole in the tests and should
> > > > > be addressed to prevent regressions.
> > > >
> > > > In git master, there are many tests for senders of
> > > > withAllowDuplicateHeaderNames(boolean) and
> > > > setAllowDuplicateHeaderNames(boolean).
> > > >
> > > > I filled in the coverage in testGetAllowDuplicateHeaderNames() for
> > > > getAllowDuplicateHeaderNames() to reflect the _current_ behavior, right
> > > or
> > > > wrong.
> > > >
> > > > Let's reassess now git master now please. If the PR matters still, it
> > > > should be rebased on git master.
> > > >
> > > > Gary
> > > >
> > > > >
> > > > > ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > > > For additional commands, e-mail: dev-help@commons.apache.org
> > > > >
> > > > >
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > > For additional commands, e-mail: dev-help@commons.apache.org
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > > For additional commands, e-mail: dev-help@commons.apache.org
> > > >
> > > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > For additional commands, e-mail: dev-help@commons.apache.org
> > >
> > >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Alex Herbert <al...@gmail.com>.
From my investigation the behaviour from 1.0 to 1.6 was to throw on
duplicates, but ignore empty duplicates (so allowing pad columns in a
CSV).
The allowDuplicates flag was added in 1.7 to allow duplicates. DEFAULT
behaviour was true! So the behavioural compatibility was broken in
release 1.7. Note there is also an allowMissingColumnNames flag. Logic
was:
- Store column headers as we go
- For each new column header, check in the existing headers
- If present:
-- If not empty then throw if allowDuplicateHeaderNames=true
-- If empty then throw if allowMissingColumnNames=false
There is some logic failure here. It would allow the first missing
column name for example; only the second empty missing column name
would trigger an exception.
The current master has updated this logic somewhat but I think the
concept is still the same. There are tests to show that the default
enum value is ALLOW_ALL. This is the same behaviour as 1.7, but not
1.0 - 1.6.
Setting the boolean flag to true -> ALLOW_ALL.
Setting the boolean flag to false -> ALLOW_EMPTY.
So I think the default settings are correct, and the use of the old
flag flips the behaviour correctly. But when investigating I found
another issue.
I looked at the CSVFormat tests and found two fixtures missing the
@Test annotation:
testDuplicateHeaderElementsTrue()
testDuplicateHeaderElementsTrue_Deprecated()
I added these to master.
The tests for duplicate headers use the builder and setHeader("A",
"A"). So there are no tests for empty headers. Trying this throws an
exception in CSVFormat.validate for all of these:
CSVFormat.DEFAULT.builder().setAllowDuplicateHeaderNames(false).setHeader("A",
"", "B", "").build();
CSVFormat.DEFAULT.builder().setDuplicateHeaderMode(DuplicateHeaderMode.ALLOW_EMPTY).setHeader("A",
"", "B", "").build();
CSVFormat.DEFAULT.builder().setAllowDuplicateHeaderNames(false)
.setAllowMissingColumnNames(true).setHeader("A", "", "B", "").build();
So it seems the logic when building a CSVFormat with explicit headers
is different from when parsing a format in CSVParser.createHeaders().
That method has a lot of logic to set up the header map. If a
CSVFormat is created with an explicit header the behaviour is
different with respect to duplicate headers, and
allowMissingColumnNames which is not even considered in
CSVFormat.validate().
This is the current behaviour in master for the parser:
@Test
public void testEmptyColumnsAndDuplicateHeadersNotAllowed() throws Exception {
assertThrows(IllegalArgumentException.class,
() -> CSVParser.parse("a,,c,\n1,2,3,4\nw,x,y,z",
CSVFormat.DEFAULT.withHeader().withAllowDuplicateHeaderNames(false)));
}
@Test
public void testEmptyColumnsAndDuplicateHeadersNotAllowedWithMissingColumnsAllowed()
throws Exception {
try (CSVParser parser = CSVParser.parse("a,,c,\n1,2,3,4\nw,x,y,z",
CSVFormat.DEFAULT.withHeader().withAllowDuplicateHeaderNames(false).withAllowMissingColumnNames(true)))
{
// noop
}
}
So the duplicate headers cannot be considered on its own, it requires
use of the allow missing column names flag.
Also, if a CSVFormat has an explicit header then the CSVParser will
not construct a header Map<String, Integer> to allow column names to
look up column indices.
I am not sure of the best path to fix this. I would document the
CSVParser header map as not being created if the header is not read
from the CSV input (current behaviour). Then maybe change the logic in
CSVFormat.validate() to respect the ALLOW_ALL, ALLOW_EMPTY, ALLOW_NONE
duplicates choice, possibly also considering allowEmptyColumnNames
which defaults to false. Ideally we should get the same exceptions
when building a CSVFormat with explicitly set headers, and parsing a
CSV file and getting the header from the file. Currently I do not
think that is true.
Alex
On Fri, 21 Oct 2022 at 15:54, Gary Gregory <ga...@gmail.com> wrote:
>
> Ok, I don't see any conflict anymore but the PR title does not match the
> code: "CSVFormat.duplicateHeaderMode requires default DISALLOW"
>
> I am ok with the changes as they stand now but we have to resolve if the
> default should be changed.
>
> Gary
>
> On Fri, Oct 21, 2022, 09:44 <sm...@outlook.de> wrote:
>
> > Hi Gary,
> >
> > the PR did not have conflicts 30 minutes ago. So we must have encountered
> > a race condition between you updating master and me rebasing and
> > force-pushing my pr.
> > I've done that again just now. There are no conflicts.
> > At the same time I see parts of my PR have dribbled into master already,
> > so if you rather implement the changes yourself, please go ahead. I am fine
> > with closing my pr.
> >
> > regards,
> > Markus
> >
> >
> > From: Gary Gregory <ga...@gmail.com>
> > Sent: Friday, October 21, 2022 15:17
> > To: Commons Developers List <de...@commons.apache.org>
> > Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
> >
> > Hi Markus,
> >
> > The PR has conflicts and does not test its changes.
> >
> > I'd like feedback from the community on whether or not git master as it is
> > now is OK for the duplicate header behavior or if changing the default is
> > needed and if doing so is just ping-pongonging from one incompatibility to
> > another, based on previous versions.
> >
> > TY!
> >
> > Gary
> >
> > On Fri, Oct 21, 2022, 08:52 <sm...@outlook.de> wrote:
> >
> > > I've removed serialization stuff from the PR and rebased it to master
> > > https://github.com/apache/commons-csv/pull/276
> > >
> > > kind regards,
> > > Markus
> > >
> > > From: Gary D. Gregory <gg...@apache.org>
> > > Sent: Friday, October 21, 2022 14:18
> > > To: dev@commons.apache.org <de...@commons.apache.org>
> > > Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
> > >
> > > On 2022/10/20 22:56:05 Alex Herbert wrote:
> > > > On Thu, 20 Oct 2022 at 23:43, Alex Herbert <al...@gmail.com>
> > > wrote:
> > > > >
> > > > > I did not have time to track through whether this behaviour changed
> > > > > after the initial implementation of the flag. I would think not as
> > the
> > > > > original behaviour is from 1.0. This would map to:
> > > > >
> > > > > true -> ALLOW_ALL
> > > > > false -> ALLOW_EMPTY
> > > > > new -> DISALLOW
> > > > >
> > > > > Which is what we currently have in 1.10.0 RC1. Thus the PR #276 [7]
> > to
> > > > > change the use of the flag to 'false -> DISALLOW' is not maintaining
> > > > > behavioural compatibility (to 1.7, or back to 1.0).
> > > >
> > > > PS. I just verified that PR 276 changes the DuplicateHeaderMode value
> > > > for allowDuplicates=false and does not change any tests.
> > > >
> > > > So the test suite is currently not enforcing behavioural
> > > > compatibility. This seems like a glaring hole in the tests and should
> > > > be addressed to prevent regressions.
> > >
> > > In git master, there are many tests for senders of
> > > withAllowDuplicateHeaderNames(boolean) and
> > > setAllowDuplicateHeaderNames(boolean).
> > >
> > > I filled in the coverage in testGetAllowDuplicateHeaderNames() for
> > > getAllowDuplicateHeaderNames() to reflect the _current_ behavior, right
> > or
> > > wrong.
> > >
> > > Let's reassess now git master now please. If the PR matters still, it
> > > should be rebased on git master.
> > >
> > > Gary
> > >
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > > For additional commands, e-mail: dev-help@commons.apache.org
> > > >
> > > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > For additional commands, e-mail: dev-help@commons.apache.org
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > For additional commands, e-mail: dev-help@commons.apache.org
> > >
> > >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Gary Gregory <ga...@gmail.com>.
Ok, I don't see any conflict anymore but the PR title does not match the
code: "CSVFormat.duplicateHeaderMode requires default DISALLOW"
I am ok with the changes as they stand now but we have to resolve if the
default should be changed.
Gary
On Fri, Oct 21, 2022, 09:44 <sm...@outlook.de> wrote:
> Hi Gary,
>
> the PR did not have conflicts 30 minutes ago. So we must have encountered
> a race condition between you updating master and me rebasing and
> force-pushing my pr.
> I've done that again just now. There are no conflicts.
> At the same time I see parts of my PR have dribbled into master already,
> so if you rather implement the changes yourself, please go ahead. I am fine
> with closing my pr.
>
> regards,
> Markus
>
>
> From: Gary Gregory <ga...@gmail.com>
> Sent: Friday, October 21, 2022 15:17
> To: Commons Developers List <de...@commons.apache.org>
> Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
>
> Hi Markus,
>
> The PR has conflicts and does not test its changes.
>
> I'd like feedback from the community on whether or not git master as it is
> now is OK for the duplicate header behavior or if changing the default is
> needed and if doing so is just ping-pongonging from one incompatibility to
> another, based on previous versions.
>
> TY!
>
> Gary
>
> On Fri, Oct 21, 2022, 08:52 <sm...@outlook.de> wrote:
>
> > I've removed serialization stuff from the PR and rebased it to master
> > https://github.com/apache/commons-csv/pull/276
> >
> > kind regards,
> > Markus
> >
> > From: Gary D. Gregory <gg...@apache.org>
> > Sent: Friday, October 21, 2022 14:18
> > To: dev@commons.apache.org <de...@commons.apache.org>
> > Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
> >
> > On 2022/10/20 22:56:05 Alex Herbert wrote:
> > > On Thu, 20 Oct 2022 at 23:43, Alex Herbert <al...@gmail.com>
> > wrote:
> > > >
> > > > I did not have time to track through whether this behaviour changed
> > > > after the initial implementation of the flag. I would think not as
> the
> > > > original behaviour is from 1.0. This would map to:
> > > >
> > > > true -> ALLOW_ALL
> > > > false -> ALLOW_EMPTY
> > > > new -> DISALLOW
> > > >
> > > > Which is what we currently have in 1.10.0 RC1. Thus the PR #276 [7]
> to
> > > > change the use of the flag to 'false -> DISALLOW' is not maintaining
> > > > behavioural compatibility (to 1.7, or back to 1.0).
> > >
> > > PS. I just verified that PR 276 changes the DuplicateHeaderMode value
> > > for allowDuplicates=false and does not change any tests.
> > >
> > > So the test suite is currently not enforcing behavioural
> > > compatibility. This seems like a glaring hole in the tests and should
> > > be addressed to prevent regressions.
> >
> > In git master, there are many tests for senders of
> > withAllowDuplicateHeaderNames(boolean) and
> > setAllowDuplicateHeaderNames(boolean).
> >
> > I filled in the coverage in testGetAllowDuplicateHeaderNames() for
> > getAllowDuplicateHeaderNames() to reflect the _current_ behavior, right
> or
> > wrong.
> >
> > Let's reassess now git master now please. If the PR matters still, it
> > should be rebased on git master.
> >
> > Gary
> >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > For additional commands, e-mail: dev-help@commons.apache.org
> > >
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by sm...@outlook.de.
Hi Gary,
the PR did not have conflicts 30 minutes ago. So we must have encountered a race condition between you updating master and me rebasing and force-pushing my pr.
I've done that again just now. There are no conflicts.
At the same time I see parts of my PR have dribbled into master already, so if you rather implement the changes yourself, please go ahead. I am fine with closing my pr.
regards,
Markus
From: Gary Gregory <ga...@gmail.com>
Sent: Friday, October 21, 2022 15:17
To: Commons Developers List <de...@commons.apache.org>
Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Hi Markus,
The PR has conflicts and does not test its changes.
I'd like feedback from the community on whether or not git master as it is
now is OK for the duplicate header behavior or if changing the default is
needed and if doing so is just ping-pongonging from one incompatibility to
another, based on previous versions.
TY!
Gary
On Fri, Oct 21, 2022, 08:52 <sm...@outlook.de> wrote:
> I've removed serialization stuff from the PR and rebased it to master
> https://github.com/apache/commons-csv/pull/276
>
> kind regards,
> Markus
>
> From: Gary D. Gregory <gg...@apache.org>
> Sent: Friday, October 21, 2022 14:18
> To: dev@commons.apache.org <de...@commons.apache.org>
> Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
>
> On 2022/10/20 22:56:05 Alex Herbert wrote:
> > On Thu, 20 Oct 2022 at 23:43, Alex Herbert <al...@gmail.com>
> wrote:
> > >
> > > I did not have time to track through whether this behaviour changed
> > > after the initial implementation of the flag. I would think not as the
> > > original behaviour is from 1.0. This would map to:
> > >
> > > true -> ALLOW_ALL
> > > false -> ALLOW_EMPTY
> > > new -> DISALLOW
> > >
> > > Which is what we currently have in 1.10.0 RC1. Thus the PR #276 [7] to
> > > change the use of the flag to 'false -> DISALLOW' is not maintaining
> > > behavioural compatibility (to 1.7, or back to 1.0).
> >
> > PS. I just verified that PR 276 changes the DuplicateHeaderMode value
> > for allowDuplicates=false and does not change any tests.
> >
> > So the test suite is currently not enforcing behavioural
> > compatibility. This seems like a glaring hole in the tests and should
> > be addressed to prevent regressions.
>
> In git master, there are many tests for senders of
> withAllowDuplicateHeaderNames(boolean) and
> setAllowDuplicateHeaderNames(boolean).
>
> I filled in the coverage in testGetAllowDuplicateHeaderNames() for
> getAllowDuplicateHeaderNames() to reflect the _current_ behavior, right or
> wrong.
>
> Let's reassess now git master now please. If the PR matters still, it
> should be rebased on git master.
>
> Gary
>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Gary Gregory <ga...@gmail.com>.
Hi Markus,
The PR has conflicts and does not test its changes.
I'd like feedback from the community on whether or not git master as it is
now is OK for the duplicate header behavior or if changing the default is
needed and if doing so is just ping-pongonging from one incompatibility to
another, based on previous versions.
TY!
Gary
On Fri, Oct 21, 2022, 08:52 <sm...@outlook.de> wrote:
> I've removed serialization stuff from the PR and rebased it to master
> https://github.com/apache/commons-csv/pull/276
>
> kind regards,
> Markus
>
> From: Gary D. Gregory <gg...@apache.org>
> Sent: Friday, October 21, 2022 14:18
> To: dev@commons.apache.org <de...@commons.apache.org>
> Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
>
> On 2022/10/20 22:56:05 Alex Herbert wrote:
> > On Thu, 20 Oct 2022 at 23:43, Alex Herbert <al...@gmail.com>
> wrote:
> > >
> > > I did not have time to track through whether this behaviour changed
> > > after the initial implementation of the flag. I would think not as the
> > > original behaviour is from 1.0. This would map to:
> > >
> > > true -> ALLOW_ALL
> > > false -> ALLOW_EMPTY
> > > new -> DISALLOW
> > >
> > > Which is what we currently have in 1.10.0 RC1. Thus the PR #276 [7] to
> > > change the use of the flag to 'false -> DISALLOW' is not maintaining
> > > behavioural compatibility (to 1.7, or back to 1.0).
> >
> > PS. I just verified that PR 276 changes the DuplicateHeaderMode value
> > for allowDuplicates=false and does not change any tests.
> >
> > So the test suite is currently not enforcing behavioural
> > compatibility. This seems like a glaring hole in the tests and should
> > be addressed to prevent regressions.
>
> In git master, there are many tests for senders of
> withAllowDuplicateHeaderNames(boolean) and
> setAllowDuplicateHeaderNames(boolean).
>
> I filled in the coverage in testGetAllowDuplicateHeaderNames() for
> getAllowDuplicateHeaderNames() to reflect the _current_ behavior, right or
> wrong.
>
> Let's reassess now git master now please. If the PR matters still, it
> should be rebased on git master.
>
> Gary
>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by sm...@outlook.de.
I've removed serialization stuff from the PR and rebased it to master
https://github.com/apache/commons-csv/pull/276
kind regards,
Markus
From: Gary D. Gregory <gg...@apache.org>
Sent: Friday, October 21, 2022 14:18
To: dev@commons.apache.org <de...@commons.apache.org>
Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
On 2022/10/20 22:56:05 Alex Herbert wrote:
> On Thu, 20 Oct 2022 at 23:43, Alex Herbert <al...@gmail.com> wrote:
> >
> > I did not have time to track through whether this behaviour changed
> > after the initial implementation of the flag. I would think not as the
> > original behaviour is from 1.0. This would map to:
> >
> > true -> ALLOW_ALL
> > false -> ALLOW_EMPTY
> > new -> DISALLOW
> >
> > Which is what we currently have in 1.10.0 RC1. Thus the PR #276 [7] to
> > change the use of the flag to 'false -> DISALLOW' is not maintaining
> > behavioural compatibility (to 1.7, or back to 1.0).
>
> PS. I just verified that PR 276 changes the DuplicateHeaderMode value
> for allowDuplicates=false and does not change any tests.
>
> So the test suite is currently not enforcing behavioural
> compatibility. This seems like a glaring hole in the tests and should
> be addressed to prevent regressions.
In git master, there are many tests for senders of withAllowDuplicateHeaderNames(boolean) and setAllowDuplicateHeaderNames(boolean).
I filled in the coverage in testGetAllowDuplicateHeaderNames() for getAllowDuplicateHeaderNames() to reflect the _current_ behavior, right or wrong.
Let's reassess now git master now please. If the PR matters still, it should be rebased on git master.
Gary
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by "Gary D. Gregory" <gg...@apache.org>.
On 2022/10/20 22:56:05 Alex Herbert wrote:
> On Thu, 20 Oct 2022 at 23:43, Alex Herbert <al...@gmail.com> wrote:
> >
> > I did not have time to track through whether this behaviour changed
> > after the initial implementation of the flag. I would think not as the
> > original behaviour is from 1.0. This would map to:
> >
> > true -> ALLOW_ALL
> > false -> ALLOW_EMPTY
> > new -> DISALLOW
> >
> > Which is what we currently have in 1.10.0 RC1. Thus the PR #276 [7] to
> > change the use of the flag to 'false -> DISALLOW' is not maintaining
> > behavioural compatibility (to 1.7, or back to 1.0).
>
> PS. I just verified that PR 276 changes the DuplicateHeaderMode value
> for allowDuplicates=false and does not change any tests.
>
> So the test suite is currently not enforcing behavioural
> compatibility. This seems like a glaring hole in the tests and should
> be addressed to prevent regressions.
In git master, there are many tests for senders of withAllowDuplicateHeaderNames(boolean) and setAllowDuplicateHeaderNames(boolean).
I filled in the coverage in testGetAllowDuplicateHeaderNames() for getAllowDuplicateHeaderNames() to reflect the _current_ behavior, right or wrong.
Let's reassess now git master now please. If the PR matters still, it should be rebased on git master.
Gary
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by David Dellsperger <da...@gmail.com>.
I had just started to look into this and was going to call out the same
thing. I'm concerned with those changes, especially the ones regarding the
allowDuplicates change, I made a note in my ticket for work to make sure we
have appropriate test cases on our end, with the RC, we didn't see any
issues with the compatibility between 1.9.0 and 1.10.0.RC.
David
On Thu, Oct 20, 2022 at 5:56 PM Alex Herbert <al...@gmail.com>
wrote:
> On Thu, 20 Oct 2022 at 23:43, Alex Herbert <al...@gmail.com>
> wrote:
> >
> > I did not have time to track through whether this behaviour changed
> > after the initial implementation of the flag. I would think not as the
> > original behaviour is from 1.0. This would map to:
> >
> > true -> ALLOW_ALL
> > false -> ALLOW_EMPTY
> > new -> DISALLOW
> >
> > Which is what we currently have in 1.10.0 RC1. Thus the PR #276 [7] to
> > change the use of the flag to 'false -> DISALLOW' is not maintaining
> > behavioural compatibility (to 1.7, or back to 1.0).
>
> PS. I just verified that PR 276 changes the DuplicateHeaderMode value
> for allowDuplicates=false and does not change any tests.
>
> So the test suite is currently not enforcing behavioural
> compatibility. This seems like a glaring hole in the tests and should
> be addressed to prevent regressions.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Alex Herbert <al...@gmail.com>.
On Thu, 20 Oct 2022 at 23:43, Alex Herbert <al...@gmail.com> wrote:
>
> I did not have time to track through whether this behaviour changed
> after the initial implementation of the flag. I would think not as the
> original behaviour is from 1.0. This would map to:
>
> true -> ALLOW_ALL
> false -> ALLOW_EMPTY
> new -> DISALLOW
>
> Which is what we currently have in 1.10.0 RC1. Thus the PR #276 [7] to
> change the use of the flag to 'false -> DISALLOW' is not maintaining
> behavioural compatibility (to 1.7, or back to 1.0).
PS. I just verified that PR 276 changes the DuplicateHeaderMode value
for allowDuplicates=false and does not change any tests.
So the test suite is currently not enforcing behavioural
compatibility. This seems like a glaring hole in the tests and should
be addressed to prevent regressions.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Alex Herbert <al...@gmail.com>.
On Thu, 20 Oct 2022 at 22:45, Gary D. Gregory <gg...@apache.org> wrote:
>
> Hi All (below)
>
> On 2022/10/20 18:08:31 Alex Herbert wrote:
> > On Thu, 20 Oct 2022 at 17:05, sebb <se...@gmail.com> wrote:
> > >
> > > On Thu, 20 Oct 2022 at 15:43, Gary Gregory <ga...@gmail.com> wrote:
> > > >
> > > > Would't it be simpler to deal with the serialization issue by bumping the
> > > > serialVersionID? We can just say that you only serialized and deserialize
> > > > for the same version.
> > >
> > > Are we willing to continue supporting serialisation going forward?
> > > It is not easy to ensure compatibility and avoid security issues.
> > >
> > > Are there any use-cases for allowing serialisation?
> >
> > Serialisation was broken for CSVRecord before and partially fixed in
> > 1.8 to support 1.0 to 1.6. Fields from 1.7 are not supported. The
> > release notes for 1.8 state that serialisation will not be supported
> > going forward. So this breakage of serialisation for CSVFormat has
> > precedent.
> >
> > I think Gary's suggestion to change the serial version ID commits to
> > the same path as not supporting serialisation from 2.0.
>
> I think this is the simplest solution. We can Javadoc the class and say that we do not support serialization from one version to the next and that it will be removed in 2.0. Check? If this OK, then I'll update git master and we can move on to the duplicate headers enum.
+1
>
> >
> > Regarding the release, I am not concerned about serialisation as it
> > seems to be a lost cause. The issue is the behavioural compatibility
> > of switching from a boolean flag for duplicate headers to an enum with
> > 3 options. We should get this correct to avoid a future release having
> > to explain a behavioural compatibility change.
>
> Duplicate headers enum: We are leaning towards canceling RC1, updating the PR, or creating a new PR. I'll wait to cancel RC1 until it is clear what is being proposed, with a PR.
I think we need to check what happened before we had the duplicate
headers flag. This is what I have found:
CSV-239 added the flag (1.7.0) [1] in commit [2]. This was added to
allow the CSVRecord getHeaderNames to return all headers including
repeats. Before that duplicates threw an exception (see CSV-236 [3],
which predates CSV-239). Throwing an exception for duplicate headers
is mentioned in the changes log for release 1.0 [5]. Note the original
behavior was to throw for non-empty duplicates due to a fix
implemented in CSV-121 for release 1.0 [6]. So this is the original
behaviour.
CSV-264 added the enum (1.10.0) [4].
So if the duplicate headers flag has been in since 1.7 then we should
just map the behaviour to the new enum. The commit when the boolean
flag was added has this text in CSVParser:
"This will always allow a duplicate header if the header is empty"
So behaviour when the flag was added:
true - allow duplicates
false - only allow empty duplicates, throw for non-empy duplicates
I did not have time to track through whether this behaviour changed
after the initial implementation of the flag. I would think not as the
original behaviour is from 1.0. This would map to:
true -> ALLOW_ALL
false -> ALLOW_EMPTY
new -> DISALLOW
Which is what we currently have in 1.10.0 RC1. Thus the PR #276 [7] to
change the use of the flag to 'false -> DISALLOW' is not maintaining
behavioural compatibility (to 1.7, or back to 1.0).
The original review by Markus Span also found inconsistent settings of
the quotedNullString. But this was removed from PR #276 and I lost
track of whether that change was required.
Alex
[1] https://issues.apache.org/jira/browse/CSV-236
[2] https://github.com/apache/commons-csv/commit/030fb8e37c4024b24fac2b5404300449a6741699
[3] https://issues.apache.org/jira/browse/CSV-236
[4] https://issues.apache.org/jira/browse/CSV-264
[5] https://commons.apache.org/proper/commons-csv/changes-report.html
[6] https://issues.apache.org/jira/browse/CSV-121
[7] https://github.com/apache/commons-csv/pull/276
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by "Gary D. Gregory" <gg...@apache.org>.
Hi All (below)
On 2022/10/20 18:08:31 Alex Herbert wrote:
> On Thu, 20 Oct 2022 at 17:05, sebb <se...@gmail.com> wrote:
> >
> > On Thu, 20 Oct 2022 at 15:43, Gary Gregory <ga...@gmail.com> wrote:
> > >
> > > Would't it be simpler to deal with the serialization issue by bumping the
> > > serialVersionID? We can just say that you only serialized and deserialize
> > > for the same version.
> >
> > Are we willing to continue supporting serialisation going forward?
> > It is not easy to ensure compatibility and avoid security issues.
> >
> > Are there any use-cases for allowing serialisation?
>
> Serialisation was broken for CSVRecord before and partially fixed in
> 1.8 to support 1.0 to 1.6. Fields from 1.7 are not supported. The
> release notes for 1.8 state that serialisation will not be supported
> going forward. So this breakage of serialisation for CSVFormat has
> precedent.
>
> I think Gary's suggestion to change the serial version ID commits to
> the same path as not supporting serialisation from 2.0.
I think this is the simplest solution. We can Javadoc the class and say that we do not support serialization from one version to the next and that it will be removed in 2.0. Check? If this OK, then I'll update git master and we can move on to the duplicate headers enum.
>
> Regarding the release, I am not concerned about serialisation as it
> seems to be a lost cause. The issue is the behavioural compatibility
> of switching from a boolean flag for duplicate headers to an enum with
> 3 options. We should get this correct to avoid a future release having
> to explain a behavioural compatibility change.
Duplicate headers enum: We are leaning towards canceling RC1, updating the PR, or creating a new PR. I'll wait to cancel RC1 until it is clear what is being proposed, with a PR.
Gary
>
> Alex
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Alex Herbert <al...@gmail.com>.
On Thu, 20 Oct 2022 at 17:05, sebb <se...@gmail.com> wrote:
>
> On Thu, 20 Oct 2022 at 15:43, Gary Gregory <ga...@gmail.com> wrote:
> >
> > Would't it be simpler to deal with the serialization issue by bumping the
> > serialVersionID? We can just say that you only serialized and deserialize
> > for the same version.
>
> Are we willing to continue supporting serialisation going forward?
> It is not easy to ensure compatibility and avoid security issues.
>
> Are there any use-cases for allowing serialisation?
Serialisation was broken for CSVRecord before and partially fixed in
1.8 to support 1.0 to 1.6. Fields from 1.7 are not supported. The
release notes for 1.8 state that serialisation will not be supported
going forward. So this breakage of serialisation for CSVFormat has
precedent.
I think Gary's suggestion to change the serial version ID commits to
the same path as not supporting serialisation from 2.0.
Regarding the release, I am not concerned about serialisation as it
seems to be a lost cause. The issue is the behavioural compatibility
of switching from a boolean flag for duplicate headers to an enum with
3 options. We should get this correct to avoid a future release having
to explain a behavioural compatibility change.
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by sebb <se...@gmail.com>.
On Thu, 20 Oct 2022 at 15:43, Gary Gregory <ga...@gmail.com> wrote:
>
> Would't it be simpler to deal with the serialization issue by bumping the
> serialVersionID? We can just say that you only serialized and deserialize
> for the same version.
Are we willing to continue supporting serialisation going forward?
It is not easy to ensure compatibility and avoid security issues.
Are there any use-cases for allowing serialisation?
> Also note the PR will throw an NPE in the builder
> when instead of using the validate() method.
>
> Gary
>
> On Wed, Oct 19, 2022, 18:27 Gary D. Gregory <gg...@apache.org> wrote:
>
> > I've commented on the PR.
> > TY.
> > Gary
> >
> > On 2022/10/19 16:51:57 Gary Gregory wrote:
> > > On Wed, Oct 19, 2022 at 10:01 AM Alex Herbert <al...@gmail.com>
> > wrote:
> > > >
> > > > On Wed, 19 Oct 2022 at 14:57, Gary D. Gregory <gg...@apache.org>
> > wrote:
> > > > >
> > > > > My +1
> > > > >
> > > > > Gary
> > > >
> > > > Gary,
> > > >
> > > > PR #276 highlights a behavioural compatibility error in the 1.10.0 RC1.
> > > >
> > > > AllowDuplicates enum may be set to the incorrect value when setting
> > > > the allow duplicates boolean. Have you reviewed this? I believe it is
> > > > valid.
> > >
> > > I will re-read later tonight...
> > >
> > > Gary
> > >
> > > >
> > > > Alex
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > > For additional commands, e-mail: dev-help@commons.apache.org
> > > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > For additional commands, e-mail: dev-help@commons.apache.org
> > >
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by "Gary D. Gregory" <gg...@apache.org>.
I updated git master with Javadoc to signal that Serialization in CSVFormat is not supported from one version to the next. I also bumped the serial version ID from 1 to 2. All of this is noted in changes.xml.
Gary
On 2022/10/21 11:27:43 sebb wrote:
> On Fri, 21 Oct 2022 at 11:57, <sm...@outlook.de> wrote:
> >
> > > Would't it be simpler to deal with the serialization issue by bumping the serialVersionID?
> > simpler yes, but it's a different thing
> > The PR makes the serialized forms for commons-csv versions 1.9.0 and 1.10.0 compatible.
> >
> > Given that serialization has been broken for several versions in commons-csv and given that "fixing" it now is a labor of love which does not add much value,
> > I am in favor of throwing it out altogether now rather than in some future (major) version. Clearly inform about it in the release notes and be done with it.
>
> +1
>
> > > Also note the PR will throw an NPE in the builder
> > > when instead of using the validate() method.
> >
> > setDuplicateHeaderMode(null) is illegal and should be fail-fast IMO
> >
> > kind regards,
> > Markus
> >
> > From: Gary Gregory <ga...@gmail.com>
> > Sent: Thursday, October 20, 2022 16:43
> > To: Commons Developers List <de...@commons.apache.org>
> > Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
> >
> > Would't it be simpler to deal with the serialization issue by bumping the
> > serialVersionID? We can just say that you only serialized and deserialize
> > for the same version. Also note the PR will throw an NPE in the builder
> > when instead of using the validate() method.
> >
> > Gary
> >
> > On Wed, Oct 19, 2022, 18:27 Gary D. Gregory <gg...@apache.org> wrote:
> >
> > > I've commented on the PR.
> > > TY.
> > > Gary
> > >
> > > On 2022/10/19 16:51:57 Gary Gregory wrote:
> > > > On Wed, Oct 19, 2022 at 10:01 AM Alex Herbert <al...@gmail.com>
> > > wrote:
> > > > >
> > > > > On Wed, 19 Oct 2022 at 14:57, Gary D. Gregory <gg...@apache.org>
> > > wrote:
> > > > > >
> > > > > > My +1
> > > > > >
> > > > > > Gary
> > > > >
> > > > > Gary,
> > > > >
> > > > > PR #276 highlights a behavioural compatibility error in the 1.10.0 RC1.
> > > > >
> > > > > AllowDuplicates enum may be set to the incorrect value when setting
> > > > > the allow duplicates boolean. Have you reviewed this? I believe it is
> > > > > valid.
> > > >
> > > > I will re-read later tonight...
> > > >
> > > > Gary
> > > >
> > > > >
> > > > > Alex
> > > > >
> > > > > ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > > > For additional commands, e-mail: dev-help@commons.apache.org
> > > > >
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > > For additional commands, e-mail: dev-help@commons.apache.org
> > > >
> > > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > For additional commands, e-mail: dev-help@commons.apache.org
> > >
> > >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by sebb <se...@gmail.com>.
On Fri, 21 Oct 2022 at 11:57, <sm...@outlook.de> wrote:
>
> > Would't it be simpler to deal with the serialization issue by bumping the serialVersionID?
> simpler yes, but it's a different thing
> The PR makes the serialized forms for commons-csv versions 1.9.0 and 1.10.0 compatible.
>
> Given that serialization has been broken for several versions in commons-csv and given that "fixing" it now is a labor of love which does not add much value,
> I am in favor of throwing it out altogether now rather than in some future (major) version. Clearly inform about it in the release notes and be done with it.
+1
> > Also note the PR will throw an NPE in the builder
> > when instead of using the validate() method.
>
> setDuplicateHeaderMode(null) is illegal and should be fail-fast IMO
>
> kind regards,
> Markus
>
> From: Gary Gregory <ga...@gmail.com>
> Sent: Thursday, October 20, 2022 16:43
> To: Commons Developers List <de...@commons.apache.org>
> Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
>
> Would't it be simpler to deal with the serialization issue by bumping the
> serialVersionID? We can just say that you only serialized and deserialize
> for the same version. Also note the PR will throw an NPE in the builder
> when instead of using the validate() method.
>
> Gary
>
> On Wed, Oct 19, 2022, 18:27 Gary D. Gregory <gg...@apache.org> wrote:
>
> > I've commented on the PR.
> > TY.
> > Gary
> >
> > On 2022/10/19 16:51:57 Gary Gregory wrote:
> > > On Wed, Oct 19, 2022 at 10:01 AM Alex Herbert <al...@gmail.com>
> > wrote:
> > > >
> > > > On Wed, 19 Oct 2022 at 14:57, Gary D. Gregory <gg...@apache.org>
> > wrote:
> > > > >
> > > > > My +1
> > > > >
> > > > > Gary
> > > >
> > > > Gary,
> > > >
> > > > PR #276 highlights a behavioural compatibility error in the 1.10.0 RC1.
> > > >
> > > > AllowDuplicates enum may be set to the incorrect value when setting
> > > > the allow duplicates boolean. Have you reviewed this? I believe it is
> > > > valid.
> > >
> > > I will re-read later tonight...
> > >
> > > Gary
> > >
> > > >
> > > > Alex
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > > For additional commands, e-mail: dev-help@commons.apache.org
> > > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > For additional commands, e-mail: dev-help@commons.apache.org
> > >
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by sm...@outlook.de.
> Would't it be simpler to deal with the serialization issue by bumping the serialVersionID?
simpler yes, but it's a different thing
The PR makes the serialized forms for commons-csv versions 1.9.0 and 1.10.0 compatible.
Given that serialization has been broken for several versions in commons-csv and given that "fixing" it now is a labor of love which does not add much value,
I am in favor of throwing it out altogether now rather than in some future (major) version. Clearly inform about it in the release notes and be done with it.
> Also note the PR will throw an NPE in the builder
> when instead of using the validate() method.
setDuplicateHeaderMode(null) is illegal and should be fail-fast IMO
kind regards,
Markus
From: Gary Gregory <ga...@gmail.com>
Sent: Thursday, October 20, 2022 16:43
To: Commons Developers List <de...@commons.apache.org>
Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Would't it be simpler to deal with the serialization issue by bumping the
serialVersionID? We can just say that you only serialized and deserialize
for the same version. Also note the PR will throw an NPE in the builder
when instead of using the validate() method.
Gary
On Wed, Oct 19, 2022, 18:27 Gary D. Gregory <gg...@apache.org> wrote:
> I've commented on the PR.
> TY.
> Gary
>
> On 2022/10/19 16:51:57 Gary Gregory wrote:
> > On Wed, Oct 19, 2022 at 10:01 AM Alex Herbert <al...@gmail.com>
> wrote:
> > >
> > > On Wed, 19 Oct 2022 at 14:57, Gary D. Gregory <gg...@apache.org>
> wrote:
> > > >
> > > > My +1
> > > >
> > > > Gary
> > >
> > > Gary,
> > >
> > > PR #276 highlights a behavioural compatibility error in the 1.10.0 RC1.
> > >
> > > AllowDuplicates enum may be set to the incorrect value when setting
> > > the allow duplicates boolean. Have you reviewed this? I believe it is
> > > valid.
> >
> > I will re-read later tonight...
> >
> > Gary
> >
> > >
> > > Alex
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > For additional commands, e-mail: dev-help@commons.apache.org
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Gary Gregory <ga...@gmail.com>.
Would't it be simpler to deal with the serialization issue by bumping the
serialVersionID? We can just say that you only serialized and deserialize
for the same version. Also note the PR will throw an NPE in the builder
when instead of using the validate() method.
Gary
On Wed, Oct 19, 2022, 18:27 Gary D. Gregory <gg...@apache.org> wrote:
> I've commented on the PR.
> TY.
> Gary
>
> On 2022/10/19 16:51:57 Gary Gregory wrote:
> > On Wed, Oct 19, 2022 at 10:01 AM Alex Herbert <al...@gmail.com>
> wrote:
> > >
> > > On Wed, 19 Oct 2022 at 14:57, Gary D. Gregory <gg...@apache.org>
> wrote:
> > > >
> > > > My +1
> > > >
> > > > Gary
> > >
> > > Gary,
> > >
> > > PR #276 highlights a behavioural compatibility error in the 1.10.0 RC1.
> > >
> > > AllowDuplicates enum may be set to the incorrect value when setting
> > > the allow duplicates boolean. Have you reviewed this? I believe it is
> > > valid.
> >
> > I will re-read later tonight...
> >
> > Gary
> >
> > >
> > > Alex
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > For additional commands, e-mail: dev-help@commons.apache.org
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by "Gary D. Gregory" <gg...@apache.org>.
I've commented on the PR.
TY.
Gary
On 2022/10/19 16:51:57 Gary Gregory wrote:
> On Wed, Oct 19, 2022 at 10:01 AM Alex Herbert <al...@gmail.com> wrote:
> >
> > On Wed, 19 Oct 2022 at 14:57, Gary D. Gregory <gg...@apache.org> wrote:
> > >
> > > My +1
> > >
> > > Gary
> >
> > Gary,
> >
> > PR #276 highlights a behavioural compatibility error in the 1.10.0 RC1.
> >
> > AllowDuplicates enum may be set to the incorrect value when setting
> > the allow duplicates boolean. Have you reviewed this? I believe it is
> > valid.
>
> I will re-read later tonight...
>
> Gary
>
> >
> > Alex
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Gary Gregory <ga...@gmail.com>.
On Wed, Oct 19, 2022 at 10:01 AM Alex Herbert <al...@gmail.com> wrote:
>
> On Wed, 19 Oct 2022 at 14:57, Gary D. Gregory <gg...@apache.org> wrote:
> >
> > My +1
> >
> > Gary
>
> Gary,
>
> PR #276 highlights a behavioural compatibility error in the 1.10.0 RC1.
>
> AllowDuplicates enum may be set to the incorrect value when setting
> the allow duplicates boolean. Have you reviewed this? I believe it is
> valid.
I will re-read later tonight...
Gary
>
> Alex
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Alex Herbert <al...@gmail.com>.
On Wed, 19 Oct 2022 at 14:57, Gary D. Gregory <gg...@apache.org> wrote:
>
> My +1
>
> Gary
Gary,
PR #276 highlights a behavioural compatibility error in the 1.10.0 RC1.
AllowDuplicates enum may be set to the incorrect value when setting
the allow duplicates boolean. Have you reviewed this? I believe it is
valid.
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by "Gary D. Gregory" <gg...@apache.org>.
My +1
Gary
On 2022/10/16 12:48:50 Gary Gregory wrote:
> To: dev@commons.apache.org
> We have fixed a few bugs and added some enhancements since Apache
> Commons CSV 1.9.0 was released, so I would like to release Apache
> Commons CSV 1.10.0.
>
> Apache Commons CSV 1.10.0 RC1 is available for review here:
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1 (svn
> revision 57404)
>
> The Git tag commons-csv-1.10.0-RC1 commit for this RC is
> 1bd1fd8e6065da9d07b5a3a1723b059246b14001 which you can browse here:
> https://gitbox.apache.org/repos/asf?p=commons-csv.git;a=commit;h=1bd1fd8e6065da9d07b5a3a1723b059246b14001
> You may checkout this tag using:
> git clone https://gitbox.apache.org/repos/asf/commons-csv.git
> --branch commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
>
> Maven artifacts are here:
> https://repository.apache.org/content/repositories/orgapachecommons-1600/org/apache/commons/commons-csv/1.10.0/
>
> These are the artifacts and their hashes:
>
> #Release SHA-512s
> #Sun Oct 16 08:32:39 EDT 2022
> Apache\ Commons\
> CSV-1.10.0.spdx.rdf.xml=c8cf7495637ce5282c72aa8e8ef4b93f206fbbdf21c7b337a34daa3b89af9816073d0185118cab4cdbed5104cc7f090e81efb614594079b02829c96829fe8aeb
> commons-csv-1.10.0-bin.tar.gz=5b0ff3f5cc4aeff6c2aa2c11e92ea8b1b1fbbcd098ba79d97bcfd9af3ea08a9ebc922e5237bda8abcb238eb0d2fe7a5ef27534d0d796a1fc77691684a698f3d1
> commons-csv-1.10.0-bin.zip=a52caa6ccda5b830c133c965fd6843d8f960e1ce3ef83decf5435022481746093cf8b4308c1fded21a168b751724b4a9f5e72f3eca34381828968bff89e7239b
> commons-csv-1.10.0-bom.json=0e3e6652e520afb697461a3f6b33458dc8ef1dd157c2b1442664b8f12f7504226916e64960cea5cc92dd2b923d18047ed921be60eb7febfbeffc8ac3e3254fc0
> commons-csv-1.10.0-bom.xml=91e494b94eea35ee7d85c8d8dbdc5029f981e0c5a8b1aae2b0f3cd77d1cea0fb0fc8c857d18d41ca055d12c4c4eb1adf70582f178a88751b836861c3286283db
> commons-csv-1.10.0-javadoc.jar=719656f5399e0889af2e8b40f49458d0dec6e8ae85413f36caa2187235c68d9e42502bf07c66c0bcc7c5c3427bc0aac51f9897581842d303501bbff5ec73bdc0
> commons-csv-1.10.0-sources.jar=c78c8fa112ef35d6d6c061b05e08816ce516a3acd56d720c59dbdaeb9d1202b16294233bd2aa2fac91cfa759456f8d7e08951876d85decdf0b8fb215a8e31754
> commons-csv-1.10.0-src.tar.gz=a0d3a7aedab567aaa32a3dd1ceddeff412e686f3fd2b0805993db7e3dd4804b4d5d59e70f1500f23e8d0ba6531832e9f20d0c298bd443d9825724d5d1b1c87b3
> commons-csv-1.10.0-src.zip=5cbdbcf367c9dd78e76228817592d70378b3cd8d919fdcd7df5c3eb2d7ebd8bd47fbb17376a631efb640be4115d42d3cc7aafa866bfefdfe073d5a4fcf625e0b
> commons-csv-1.10.0-test-sources.jar=0d77c791934609b176457df5c60fdd5bfa3c6f392375adef81e5e7cd31de7fb25bd91b0bbe60a9ff46acb62efc2a33660adff24850370b7546a71374aa267f94
> commons-csv-1.10.0-tests.jar=9a08e4d2ec865d9e1dafc01e4ee58804490b2d50f886bac47a5235581077b31a15dda5b3f8f2adb5a70be8794cca886126d980d6d3722eec2e10471fc24e1940
>
> I have tested this with 'mvn' and 'mvn -V -Duser.name=$my_apache_id
> -Prelease -Ptest-deploy -P jacoco -P japicmp clean package site
> deploy' using:
>
> Darwin *** 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:17:10
> PDT 2022; root:xnu-8020.140.49~2/RELEASE_X86_64 x86_64
>
> Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)
> Maven home: /usr/local/Cellar/maven/3.8.6/libexec
> Java version: 1.8.0_345, vendor: Homebrew, runtime:
> /usr/local/Cellar/openjdk@8/1.8.0+345/libexec/openjdk.jdk/Contents/Home/jre
> Default locale: en_US, platform encoding: UTF-8
> OS name: "mac os x", version: "12.6", arch: "x86_64", family: "mac"
>
> Details of changes since 1.9.0 are in the release notes:
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/RELEASE-NOTES.txt
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/changes-report.html
>
> Site:
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/index.html
> (note some *relative* links are broken and the 1.10.0 directories
> are not yet created - these will be OK once the site is deployed.)
>
> JApiCmp Report (compared to 1.9.0):
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/japicmp.html
>
> RAT Report:
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/rat-report.html
>
> KEYS:
> https://downloads.apache.org/commons/KEYS
>
> Please review the release candidate and vote.
> This vote will close no sooner than 72 hours from now.
>
> [ ] +1 Release these artifacts
> [ ] +0 OK, but...
> [ ] -0 OK, but really should fix...
> [ ] -1 I oppose this release because...
>
> Thank you,
>
> Gary Gregory,
> Release Manager (using key 86fdc7e2a11262cb)
>
> For following is intended as a helper and refresher for reviewers.
>
> Validating a release candidate
> ==============================
>
> These guidelines are NOT complete.
>
> Requirements: Git, Java, Maven.
>
> You can validate a release from a release candidate (RC) tag as follows.
>
> 1) Clone and checkout the RC tag
>
> git clone https://gitbox.apache.org/repos/asf/commons-csv.git --branch
> commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
> cd commons-csv-1.10.0-RC1
>
> 2) Check Apache licenses
>
> This step is not required if the site includes a RAT report page which
> you then must check.
>
> mvn apache-rat:check
>
> 3) Check binary compatibility
>
> Use JApiCmp with the japicmp Maven Profile:
>
> This step is not required if the site includes a JApiCmp report page
> which you then must check.
>
> mvn install -DskipTests -P japicmp japicmp:cmp
>
> 4) Build the package
>
> mvn -V clean package
>
> You can record the Maven and Java version produced by -V in your VOTE reply.
> To gather OS information from a command line:
> Windows: ver
> Linux: uname -a
>
> 5) Build the site for a single module project
>
> Note: Some plugins require the components to be installed instead of packaged.
>
> mvn site
> Check the site reports in:
> - Windows: target\site\index.html
> - Linux: target/site/index.html
>
> -the end-
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[CANCEL][VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Gary Gregory <ga...@gmail.com>.
This vote is canceled, RC2 coming up...
Gary
On Sun, Oct 16, 2022 at 8:48 AM Gary Gregory <ga...@gmail.com> wrote:
>
> To: dev@commons.apache.org
> We have fixed a few bugs and added some enhancements since Apache
> Commons CSV 1.9.0 was released, so I would like to release Apache
> Commons CSV 1.10.0.
>
> Apache Commons CSV 1.10.0 RC1 is available for review here:
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1 (svn
> revision 57404)
>
> The Git tag commons-csv-1.10.0-RC1 commit for this RC is
> 1bd1fd8e6065da9d07b5a3a1723b059246b14001 which you can browse here:
> https://gitbox.apache.org/repos/asf?p=commons-csv.git;a=commit;h=1bd1fd8e6065da9d07b5a3a1723b059246b14001
> You may checkout this tag using:
> git clone https://gitbox.apache.org/repos/asf/commons-csv.git
> --branch commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
>
> Maven artifacts are here:
> https://repository.apache.org/content/repositories/orgapachecommons-1600/org/apache/commons/commons-csv/1.10.0/
>
> These are the artifacts and their hashes:
>
> #Release SHA-512s
> #Sun Oct 16 08:32:39 EDT 2022
> Apache\ Commons\
> CSV-1.10.0.spdx.rdf.xml=c8cf7495637ce5282c72aa8e8ef4b93f206fbbdf21c7b337a34daa3b89af9816073d0185118cab4cdbed5104cc7f090e81efb614594079b02829c96829fe8aeb
> commons-csv-1.10.0-bin.tar.gz=5b0ff3f5cc4aeff6c2aa2c11e92ea8b1b1fbbcd098ba79d97bcfd9af3ea08a9ebc922e5237bda8abcb238eb0d2fe7a5ef27534d0d796a1fc77691684a698f3d1
> commons-csv-1.10.0-bin.zip=a52caa6ccda5b830c133c965fd6843d8f960e1ce3ef83decf5435022481746093cf8b4308c1fded21a168b751724b4a9f5e72f3eca34381828968bff89e7239b
> commons-csv-1.10.0-bom.json=0e3e6652e520afb697461a3f6b33458dc8ef1dd157c2b1442664b8f12f7504226916e64960cea5cc92dd2b923d18047ed921be60eb7febfbeffc8ac3e3254fc0
> commons-csv-1.10.0-bom.xml=91e494b94eea35ee7d85c8d8dbdc5029f981e0c5a8b1aae2b0f3cd77d1cea0fb0fc8c857d18d41ca055d12c4c4eb1adf70582f178a88751b836861c3286283db
> commons-csv-1.10.0-javadoc.jar=719656f5399e0889af2e8b40f49458d0dec6e8ae85413f36caa2187235c68d9e42502bf07c66c0bcc7c5c3427bc0aac51f9897581842d303501bbff5ec73bdc0
> commons-csv-1.10.0-sources.jar=c78c8fa112ef35d6d6c061b05e08816ce516a3acd56d720c59dbdaeb9d1202b16294233bd2aa2fac91cfa759456f8d7e08951876d85decdf0b8fb215a8e31754
> commons-csv-1.10.0-src.tar.gz=a0d3a7aedab567aaa32a3dd1ceddeff412e686f3fd2b0805993db7e3dd4804b4d5d59e70f1500f23e8d0ba6531832e9f20d0c298bd443d9825724d5d1b1c87b3
> commons-csv-1.10.0-src.zip=5cbdbcf367c9dd78e76228817592d70378b3cd8d919fdcd7df5c3eb2d7ebd8bd47fbb17376a631efb640be4115d42d3cc7aafa866bfefdfe073d5a4fcf625e0b
> commons-csv-1.10.0-test-sources.jar=0d77c791934609b176457df5c60fdd5bfa3c6f392375adef81e5e7cd31de7fb25bd91b0bbe60a9ff46acb62efc2a33660adff24850370b7546a71374aa267f94
> commons-csv-1.10.0-tests.jar=9a08e4d2ec865d9e1dafc01e4ee58804490b2d50f886bac47a5235581077b31a15dda5b3f8f2adb5a70be8794cca886126d980d6d3722eec2e10471fc24e1940
>
> I have tested this with 'mvn' and 'mvn -V -Duser.name=$my_apache_id
> -Prelease -Ptest-deploy -P jacoco -P japicmp clean package site
> deploy' using:
>
> Darwin *** 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:17:10
> PDT 2022; root:xnu-8020.140.49~2/RELEASE_X86_64 x86_64
>
> Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)
> Maven home: /usr/local/Cellar/maven/3.8.6/libexec
> Java version: 1.8.0_345, vendor: Homebrew, runtime:
> /usr/local/Cellar/openjdk@8/1.8.0+345/libexec/openjdk.jdk/Contents/Home/jre
> Default locale: en_US, platform encoding: UTF-8
> OS name: "mac os x", version: "12.6", arch: "x86_64", family: "mac"
>
> Details of changes since 1.9.0 are in the release notes:
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/RELEASE-NOTES.txt
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/changes-report.html
>
> Site:
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/index.html
> (note some *relative* links are broken and the 1.10.0 directories
> are not yet created - these will be OK once the site is deployed.)
>
> JApiCmp Report (compared to 1.9.0):
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/japicmp.html
>
> RAT Report:
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/rat-report.html
>
> KEYS:
> https://downloads.apache.org/commons/KEYS
>
> Please review the release candidate and vote.
> This vote will close no sooner than 72 hours from now.
>
> [ ] +1 Release these artifacts
> [ ] +0 OK, but...
> [ ] -0 OK, but really should fix...
> [ ] -1 I oppose this release because...
>
> Thank you,
>
> Gary Gregory,
> Release Manager (using key 86fdc7e2a11262cb)
>
> For following is intended as a helper and refresher for reviewers.
>
> Validating a release candidate
> ==============================
>
> These guidelines are NOT complete.
>
> Requirements: Git, Java, Maven.
>
> You can validate a release from a release candidate (RC) tag as follows.
>
> 1) Clone and checkout the RC tag
>
> git clone https://gitbox.apache.org/repos/asf/commons-csv.git --branch
> commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
> cd commons-csv-1.10.0-RC1
>
> 2) Check Apache licenses
>
> This step is not required if the site includes a RAT report page which
> you then must check.
>
> mvn apache-rat:check
>
> 3) Check binary compatibility
>
> Use JApiCmp with the japicmp Maven Profile:
>
> This step is not required if the site includes a JApiCmp report page
> which you then must check.
>
> mvn install -DskipTests -P japicmp japicmp:cmp
>
> 4) Build the package
>
> mvn -V clean package
>
> You can record the Maven and Java version produced by -V in your VOTE reply.
> To gather OS information from a command line:
> Windows: ver
> Linux: uname -a
>
> 5) Build the site for a single module project
>
> Note: Some plugins require the components to be installed instead of packaged.
>
> mvn site
> Check the site reports in:
> - Windows: target\site\index.html
> - Linux: target/site/index.html
>
> -the end-
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Bruno Kinoshita <ki...@apache.org>.
+1
Built successfully from tag, using:
Apache Maven 3.8.5 (3599d3414f046de2324203b78ddcf9b5e4388aa0)
Maven home: /opt/apache-maven-3.8.5
Java version: 17.0.4, vendor: Private Build, runtime:
/usr/lib/jvm/java-17-openjdk-amd64
Default locale: en_US, platform encoding: UTF-8
OS name: "linux", version: "5.15.0-50-generic", arch: "amd64", family:
"unix"
Cheers
Bruno
On Mon, 17 Oct 2022 at 01:49, Gary Gregory <ga...@gmail.com> wrote:
> To: dev@commons.apache.org
> We have fixed a few bugs and added some enhancements since Apache
> Commons CSV 1.9.0 was released, so I would like to release Apache
> Commons CSV 1.10.0.
>
> Apache Commons CSV 1.10.0 RC1 is available for review here:
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1 (svn
> revision 57404)
>
> The Git tag commons-csv-1.10.0-RC1 commit for this RC is
> 1bd1fd8e6065da9d07b5a3a1723b059246b14001 which you can browse here:
>
> https://gitbox.apache.org/repos/asf?p=commons-csv.git;a=commit;h=1bd1fd8e6065da9d07b5a3a1723b059246b14001
> You may checkout this tag using:
> git clone https://gitbox.apache.org/repos/asf/commons-csv.git
> --branch <https://gitbox.apache.org/repos/asf/commons-csv.git--branch>
> commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
>
> Maven artifacts are here:
>
> https://repository.apache.org/content/repositories/orgapachecommons-1600/org/apache/commons/commons-csv/1.10.0/
>
> These are the artifacts and their hashes:
>
> #Release SHA-512s
> #Sun Oct 16 08:32:39 EDT 2022
> Apache\ Commons\
>
> CSV-1.10.0.spdx.rdf.xml=c8cf7495637ce5282c72aa8e8ef4b93f206fbbdf21c7b337a34daa3b89af9816073d0185118cab4cdbed5104cc7f090e81efb614594079b02829c96829fe8aeb
>
> commons-csv-1.10.0-bin.tar.gz=5b0ff3f5cc4aeff6c2aa2c11e92ea8b1b1fbbcd098ba79d97bcfd9af3ea08a9ebc922e5237bda8abcb238eb0d2fe7a5ef27534d0d796a1fc77691684a698f3d1
>
> commons-csv-1.10.0-bin.zip=a52caa6ccda5b830c133c965fd6843d8f960e1ce3ef83decf5435022481746093cf8b4308c1fded21a168b751724b4a9f5e72f3eca34381828968bff89e7239b
>
> commons-csv-1.10.0-bom.json=0e3e6652e520afb697461a3f6b33458dc8ef1dd157c2b1442664b8f12f7504226916e64960cea5cc92dd2b923d18047ed921be60eb7febfbeffc8ac3e3254fc0
>
> commons-csv-1.10.0-bom.xml=91e494b94eea35ee7d85c8d8dbdc5029f981e0c5a8b1aae2b0f3cd77d1cea0fb0fc8c857d18d41ca055d12c4c4eb1adf70582f178a88751b836861c3286283db
>
> commons-csv-1.10.0-javadoc.jar=719656f5399e0889af2e8b40f49458d0dec6e8ae85413f36caa2187235c68d9e42502bf07c66c0bcc7c5c3427bc0aac51f9897581842d303501bbff5ec73bdc0
>
> commons-csv-1.10.0-sources.jar=c78c8fa112ef35d6d6c061b05e08816ce516a3acd56d720c59dbdaeb9d1202b16294233bd2aa2fac91cfa759456f8d7e08951876d85decdf0b8fb215a8e31754
>
> commons-csv-1.10.0-src.tar.gz=a0d3a7aedab567aaa32a3dd1ceddeff412e686f3fd2b0805993db7e3dd4804b4d5d59e70f1500f23e8d0ba6531832e9f20d0c298bd443d9825724d5d1b1c87b3
>
> commons-csv-1.10.0-src.zip=5cbdbcf367c9dd78e76228817592d70378b3cd8d919fdcd7df5c3eb2d7ebd8bd47fbb17376a631efb640be4115d42d3cc7aafa866bfefdfe073d5a4fcf625e0b
>
> commons-csv-1.10.0-test-sources.jar=0d77c791934609b176457df5c60fdd5bfa3c6f392375adef81e5e7cd31de7fb25bd91b0bbe60a9ff46acb62efc2a33660adff24850370b7546a71374aa267f94
>
> commons-csv-1.10.0-tests.jar=9a08e4d2ec865d9e1dafc01e4ee58804490b2d50f886bac47a5235581077b31a15dda5b3f8f2adb5a70be8794cca886126d980d6d3722eec2e10471fc24e1940
>
> I have tested this with 'mvn' and 'mvn -V -Duser.name=$my_apache_id
> -Prelease -Ptest-deploy -P jacoco -P japicmp clean package site
> deploy' using:
>
> Darwin *** 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:17:10
> PDT 2022; root:xnu-8020.140.49~2/RELEASE_X86_64 x86_64
>
> Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)
> Maven home: /usr/local/Cellar/maven/3.8.6/libexec
> Java version: 1.8.0_345, vendor: Homebrew, runtime:
> /usr/local/Cellar/openjdk@8
> /1.8.0+345/libexec/openjdk.jdk/Contents/Home/jre
> Default locale: en_US, platform encoding: UTF-8
> OS name: "mac os x", version: "12.6", arch: "x86_64", family: "mac"
>
> Details of changes since 1.9.0 are in the release notes:
>
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/RELEASE-NOTES.txt
>
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/changes-report.html
>
> Site:
>
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/index.html
> (note some *relative* links are broken and the 1.10.0 directories
> are not yet created - these will be OK once the site is deployed.)
>
> JApiCmp Report (compared to 1.9.0):
>
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/japicmp.html
>
> RAT Report:
>
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/rat-report.html
>
> KEYS:
> https://downloads.apache.org/commons/KEYS
>
> Please review the release candidate and vote.
> This vote will close no sooner than 72 hours from now.
>
> [ ] +1 Release these artifacts
> [ ] +0 OK, but...
> [ ] -0 OK, but really should fix...
> [ ] -1 I oppose this release because...
>
> Thank you,
>
> Gary Gregory,
> Release Manager (using key 86fdc7e2a11262cb)
>
> For following is intended as a helper and refresher for reviewers.
>
> Validating a release candidate
> ==============================
>
> These guidelines are NOT complete.
>
> Requirements: Git, Java, Maven.
>
> You can validate a release from a release candidate (RC) tag as follows.
>
> 1) Clone and checkout the RC tag
>
> git clone https://gitbox.apache.org/repos/asf/commons-csv.git --branch
> commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
> cd commons-csv-1.10.0-RC1
>
> 2) Check Apache licenses
>
> This step is not required if the site includes a RAT report page which
> you then must check.
>
> mvn apache-rat:check
>
> 3) Check binary compatibility
>
> Use JApiCmp with the japicmp Maven Profile:
>
> This step is not required if the site includes a JApiCmp report page
> which you then must check.
>
> mvn install -DskipTests -P japicmp japicmp:cmp
>
> 4) Build the package
>
> mvn -V clean package
>
> You can record the Maven and Java version produced by -V in your VOTE
> reply.
> To gather OS information from a command line:
> Windows: ver
> Linux: uname -a
>
> 5) Build the site for a single module project
>
> Note: Some plugins require the components to be installed instead of
> packaged.
>
> mvn site
> Check the site reports in:
> - Windows: target\site\index.html
> - Linux: target/site/index.html
>
> -the end-
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by "Gary D. Gregory" <gg...@apache.org>.
Hi Markus,
Anyone can vote, please see https://www.apache.org/foundation/voting.html
Note that PMC member votes are binding, while others are advisory.
Gary
On 2022/10/17 10:00:13 sman81@outlook.de wrote:
> Hello
>
> CSV-264 (Add DuplicateHeaderMode) introduces bugs that should be fixed before shipping 1.10.0 IMO
> - missing default
> - broken serialization of class CSVFormat
>
> I raised these issues in CSV-302.
>
> The serialization issue is caught by Revapi. I had suggested to include Revapi in the project (CSV-303) or alternatively add revapi and drop clirr in commons-parent but that more or less fell on deaf ears.
>
> mvn org.revapi:revapi-maven-plugin:check
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD FAILURE
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 3.560 s
> [INFO] Finished at: 2022-10-17T11:30:21+02:00
> [INFO] ------------------------------------------------------------------------
> [ERROR] Failed to execute goal org.revapi:revapi-maven-plugin:0.14.7:check (default-cli) on project commons-csv: The following API problems caused the build to fail:
> [ERROR] java.field.serialVersionUIDUnchanged: field org.apache.commons.csv.CSVFormat.serialVersionUID: The class changed in an incompatible way with regards to serialization but the serialVersionUID field stayed unchanged. This might be ok and/or desired but is suspicious. https://revapi.org/revapi-java/differences.html#java.field.serialVersionUIDUnchanged
> [ERROR] java.method.exception.checkedRemoved: method java.util.List<org.apache.commons.csv.CSVRecord> org.apache.commons.csv.CSVParser::getRecords(): Method no longer throws checked exceptions: java.io.IOException. https://revapi.org/revapi-java/differences.html#java.method.exception.checkedRemoved
>
> Also noticed that class DuplicateHeaderMode and method CSVFormat.getDuplicateHeaderMode() are incorrectly annotated with "@since 1.9.0". Both are new in version 1.10.0.
>
> My vote is -1 assuming I am allowed to vote.
>
> Kind regards,
> Markus
>
> From: Gary Gregory <ga...@gmail.com>
> Sent: Sunday, October 16, 2022 14:48
> To: Commons Developers List <de...@commons.apache.org>
> Subject: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
>
> To: dev@commons.apache.org
> We have fixed a few bugs and added some enhancements since Apache
> Commons CSV 1.9.0 was released, so I would like to release Apache
> Commons CSV 1.10.0.
>
> Apache Commons CSV 1.10.0 RC1 is available for review here:
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1 (svn
> revision 57404)
>
> The Git tag commons-csv-1.10.0-RC1 commit for this RC is
> 1bd1fd8e6065da9d07b5a3a1723b059246b14001 which you can browse here:
> https://gitbox.apache.org/repos/asf?p=commons-csv.git;a=commit;h=1bd1fd8e6065da9d07b5a3a1723b059246b14001
> You may checkout this tag using:
> git clone https://gitbox.apache.org/repos/asf/commons-csv.git
> --branch commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
>
> Maven artifacts are here:
> https://repository.apache.org/content/repositories/orgapachecommons-1600/org/apache/commons/commons-csv/1.10.0/
>
> These are the artifacts and their hashes:
>
> #Release SHA-512s
> #Sun Oct 16 08:32:39 EDT 2022
> Apache\ Commons\
> CSV-1.10.0.spdx.rdf.xml=c8cf7495637ce5282c72aa8e8ef4b93f206fbbdf21c7b337a34daa3b89af9816073d0185118cab4cdbed5104cc7f090e81efb614594079b02829c96829fe8aeb
> commons-csv-1.10.0-bin.tar.gz=5b0ff3f5cc4aeff6c2aa2c11e92ea8b1b1fbbcd098ba79d97bcfd9af3ea08a9ebc922e5237bda8abcb238eb0d2fe7a5ef27534d0d796a1fc77691684a698f3d1
> commons-csv-1.10.0-bin.zip=a52caa6ccda5b830c133c965fd6843d8f960e1ce3ef83decf5435022481746093cf8b4308c1fded21a168b751724b4a9f5e72f3eca34381828968bff89e7239b
> commons-csv-1.10.0-bom.json=0e3e6652e520afb697461a3f6b33458dc8ef1dd157c2b1442664b8f12f7504226916e64960cea5cc92dd2b923d18047ed921be60eb7febfbeffc8ac3e3254fc0
> commons-csv-1.10.0-bom.xml=91e494b94eea35ee7d85c8d8dbdc5029f981e0c5a8b1aae2b0f3cd77d1cea0fb0fc8c857d18d41ca055d12c4c4eb1adf70582f178a88751b836861c3286283db
> commons-csv-1.10.0-javadoc.jar=719656f5399e0889af2e8b40f49458d0dec6e8ae85413f36caa2187235c68d9e42502bf07c66c0bcc7c5c3427bc0aac51f9897581842d303501bbff5ec73bdc0
> commons-csv-1.10.0-sources.jar=c78c8fa112ef35d6d6c061b05e08816ce516a3acd56d720c59dbdaeb9d1202b16294233bd2aa2fac91cfa759456f8d7e08951876d85decdf0b8fb215a8e31754
> commons-csv-1.10.0-src.tar.gz=a0d3a7aedab567aaa32a3dd1ceddeff412e686f3fd2b0805993db7e3dd4804b4d5d59e70f1500f23e8d0ba6531832e9f20d0c298bd443d9825724d5d1b1c87b3
> commons-csv-1.10.0-src.zip=5cbdbcf367c9dd78e76228817592d70378b3cd8d919fdcd7df5c3eb2d7ebd8bd47fbb17376a631efb640be4115d42d3cc7aafa866bfefdfe073d5a4fcf625e0b
> commons-csv-1.10.0-test-sources.jar=0d77c791934609b176457df5c60fdd5bfa3c6f392375adef81e5e7cd31de7fb25bd91b0bbe60a9ff46acb62efc2a33660adff24850370b7546a71374aa267f94
> commons-csv-1.10.0-tests.jar=9a08e4d2ec865d9e1dafc01e4ee58804490b2d50f886bac47a5235581077b31a15dda5b3f8f2adb5a70be8794cca886126d980d6d3722eec2e10471fc24e1940
>
> I have tested this with 'mvn' and 'mvn -V -Duser.name=$my_apache_id
> -Prelease -Ptest-deploy -P jacoco -P japicmp clean package site
> deploy' using:
>
> Darwin *** 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:17:10
> PDT 2022; root:xnu-8020.140.49~2/RELEASE_X86_64 x86_64
>
> Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)
> Maven home: /usr/local/Cellar/maven/3.8.6/libexec
> Java version: 1.8.0_345, vendor: Homebrew, runtime:
> /usr/local/Cellar/openjdk@8/1.8.0+345/libexec/openjdk.jdk/Contents/Home/jre
> Default locale: en_US, platform encoding: UTF-8
> OS name: "mac os x", version: "12.6", arch: "x86_64", family: "mac"
>
> Details of changes since 1.9.0 are in the release notes:
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/RELEASE-NOTES.txt
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/changes-report.html
>
> Site:
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/index.html
> (note some *relative* links are broken and the 1.10.0 directories
> are not yet created - these will be OK once the site is deployed.)
>
> JApiCmp Report (compared to 1.9.0):
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/japicmp.html
>
> RAT Report:
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/rat-report.html
>
> KEYS:
> https://downloads.apache.org/commons/KEYS
>
> Please review the release candidate and vote.
> This vote will close no sooner than 72 hours from now.
>
> [ ] +1 Release these artifacts
> [ ] +0 OK, but...
> [ ] -0 OK, but really should fix...
> [ ] -1 I oppose this release because...
>
> Thank you,
>
> Gary Gregory,
> Release Manager (using key 86fdc7e2a11262cb)
>
> For following is intended as a helper and refresher for reviewers.
>
> Validating a release candidate
> ==============================
>
> These guidelines are NOT complete.
>
> Requirements: Git, Java, Maven.
>
> You can validate a release from a release candidate (RC) tag as follows.
>
> 1) Clone and checkout the RC tag
>
> git clone https://gitbox.apache.org/repos/asf/commons-csv.git --branch
> commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
> cd commons-csv-1.10.0-RC1
>
> 2) Check Apache licenses
>
> This step is not required if the site includes a RAT report page which
> you then must check.
>
> mvn apache-rat:check
>
> 3) Check binary compatibility
>
> Use JApiCmp with the japicmp Maven Profile:
>
> This step is not required if the site includes a JApiCmp report page
> which you then must check.
>
> mvn install -DskipTests -P japicmp japicmp:cmp
>
> 4) Build the package
>
> mvn -V clean package
>
> You can record the Maven and Java version produced by -V in your VOTE reply.
> To gather OS information from a command line:
> Windows: ver
> Linux: uname -a
>
> 5) Build the site for a single module project
>
> Note: Some plugins require the components to be installed instead of packaged.
>
> mvn site
> Check the site reports in:
> - Windows: target\site\index.html
> - Linux: target/site/index.html
>
> -the end-
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Alex Herbert <al...@gmail.com>.
On Wed, 19 Oct 2022 at 14:52, <sm...@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?
Serialization is already broken for CSVRecord from 1.7+. It is broken
for CSVFormat 1.7+. It will be removed in 2.0. So I think that
serialization should be ignored going forward.
>
> Will PR #276 be part of version 1.10.0?
Perhaps it should be.
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Gary Gregory <ga...@gmail.com>.
On Wed, Oct 19, 2022 at 9:52 AM <sm...@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 <al...@gmail.com>
> Sent: Monday, October 17, 2022 19:53
> To: Commons Developers List <de...@commons.apache.org>
> Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
>
> On Mon, 17 Oct 2022 at 17:11, <sm...@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-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by sm...@outlook.de.
> 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 <al...@gmail.com>
Sent: Monday, October 17, 2022 19:53
To: Commons Developers List <de...@commons.apache.org>
Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
On Mon, 17 Oct 2022 at 17:11, <sm...@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-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Alex Herbert <al...@gmail.com>.
On Mon, 17 Oct 2022 at 17:11, <sm...@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-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by sm...@outlook.de.
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.
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.
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?
Kind regards,
Markus
________________________________
From: Alex Herbert <al...@gmail.com>
Sent: Monday, October 17, 2022 13:46
To: Commons Developers List <de...@commons.apache.org>
Subject: Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
On Mon, 17 Oct 2022 at 11:00, <sm...@outlook.de> wrote:
>
> Hello
>
> CSV-264 (Add DuplicateHeaderMode) introduces bugs that should be fixed before shipping 1.10.0 IMO
> - missing default
> - broken serialization of class CSVFormat
>
> I raised these issues in CSV-302.
>
> The serialization issue is caught by Revapi. I had suggested to include Revapi in the project (CSV-303) or alternatively add revapi and drop clirr in commons-parent but that more or less fell on deaf ears.
>
> mvn org.revapi:revapi-maven-plugin:check
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD FAILURE
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 3.560 s
> [INFO] Finished at: 2022-10-17T11:30:21+02:00
> [INFO] ------------------------------------------------------------------------
> [ERROR] Failed to execute goal org.revapi:revapi-maven-plugin:0.14.7:check (default-cli) on project commons-csv: The following API problems caused the build to fail:
> [ERROR] java.field.serialVersionUIDUnchanged: field org.apache.commons.csv.CSVFormat.serialVersionUID: The class changed in an incompatible way with regards to serialization but the serialVersionUID field stayed unchanged. This might be ok and/or desired but is suspicious. https://revapi.org/revapi-java/differences.html#java.field.serialVersionUIDUnchanged
This is an allowed source and binary compatible error; it has
potentially breaking semantic severity which do apply here.
The CSVFormat object is deserialized using the defaultReadObject
method. This will be able to deserialize older versions of CSVFormat.
The new fields added in this version will be absent from the
ObjectInputStream and set to their default initialised value.
IIUC this is the change:
remove: private boolean allowDuplicateHeaderNames
add: private DuplicateHeaderMode duplicateHeaderMode
Deserialization would mean that the new field 'duplicateHeaderMode' is
set to null and the previous field 'allowDuplicateHeaderNames' is
ignored. To fix this would require setting duplicateHeaderMode
appropriately using the field allowDuplicateHeaderName. This is the
logic from the current builder:
DuplicateHeaderMode mode = allowDuplicateHeaderNames ?
DuplicateHeaderMode.ALLOW_ALL : DuplicateHeaderMode.ALLOW_EMPTY
However, serialization was broken previously in CSV. This was for the
CSVRecord. The release notes contain this in the description for 1.8:
"This release fixes serialization compatibility of CSVRecord with
versions 1.0 to 1.6. New fields added since 1.7 are not serialized.
Support for Serializable is scheduled to be removed in version 2.0."
The Jira ticket is 248:
https://issues.apache.org/jira/browse/CSV-248
In that case we made sure we could (de)serialize without throwing an
exception to minimise errors for downstream users. But we did not fix
behavioural compatibility across versions. In this instance I do not
think exceptions need to be addressed as missing fields will be
ignored and the new field will be initialized as null. However I have
not verified this with a serialization test. If this new field is null
then the format will behave as if duplicates is set to DISALLOW.
I do not know the user case for serializing a CSVFormat. It would not
be within the library. A reference is kept in CSVParser and CSVPrinter
but these are not Serializable.
The serialization support dates back to the legacy codebase used to
create the project. Serialization can be replaced by other more
configurable libraries to read/write binary objects. Perhaps we just
need to add to the release notes that CSVFormat serialization is no
longer supported.
> [ERROR] java.method.exception.checkedRemoved: method java.util.List<org.apache.commons.csv.CSVRecord> org.apache.commons.csv.CSVParser::getRecords(): Method no longer throws checked exceptions: java.io.IOException. https://revapi.org/revapi-java/differences.html#java.method.exception.checkedRemoved
This is a source incompatible error. It is binary compatible.
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Alex Herbert <al...@gmail.com>.
On Mon, 17 Oct 2022 at 11:00, <sm...@outlook.de> wrote:
>
> Hello
>
> CSV-264 (Add DuplicateHeaderMode) introduces bugs that should be fixed before shipping 1.10.0 IMO
> - missing default
> - broken serialization of class CSVFormat
>
> I raised these issues in CSV-302.
>
> The serialization issue is caught by Revapi. I had suggested to include Revapi in the project (CSV-303) or alternatively add revapi and drop clirr in commons-parent but that more or less fell on deaf ears.
>
> mvn org.revapi:revapi-maven-plugin:check
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD FAILURE
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 3.560 s
> [INFO] Finished at: 2022-10-17T11:30:21+02:00
> [INFO] ------------------------------------------------------------------------
> [ERROR] Failed to execute goal org.revapi:revapi-maven-plugin:0.14.7:check (default-cli) on project commons-csv: The following API problems caused the build to fail:
> [ERROR] java.field.serialVersionUIDUnchanged: field org.apache.commons.csv.CSVFormat.serialVersionUID: The class changed in an incompatible way with regards to serialization but the serialVersionUID field stayed unchanged. This might be ok and/or desired but is suspicious. https://revapi.org/revapi-java/differences.html#java.field.serialVersionUIDUnchanged
This is an allowed source and binary compatible error; it has
potentially breaking semantic severity which do apply here.
The CSVFormat object is deserialized using the defaultReadObject
method. This will be able to deserialize older versions of CSVFormat.
The new fields added in this version will be absent from the
ObjectInputStream and set to their default initialised value.
IIUC this is the change:
remove: private boolean allowDuplicateHeaderNames
add: private DuplicateHeaderMode duplicateHeaderMode
Deserialization would mean that the new field 'duplicateHeaderMode' is
set to null and the previous field 'allowDuplicateHeaderNames' is
ignored. To fix this would require setting duplicateHeaderMode
appropriately using the field allowDuplicateHeaderName. This is the
logic from the current builder:
DuplicateHeaderMode mode = allowDuplicateHeaderNames ?
DuplicateHeaderMode.ALLOW_ALL : DuplicateHeaderMode.ALLOW_EMPTY
However, serialization was broken previously in CSV. This was for the
CSVRecord. The release notes contain this in the description for 1.8:
"This release fixes serialization compatibility of CSVRecord with
versions 1.0 to 1.6. New fields added since 1.7 are not serialized.
Support for Serializable is scheduled to be removed in version 2.0."
The Jira ticket is 248:
https://issues.apache.org/jira/browse/CSV-248
In that case we made sure we could (de)serialize without throwing an
exception to minimise errors for downstream users. But we did not fix
behavioural compatibility across versions. In this instance I do not
think exceptions need to be addressed as missing fields will be
ignored and the new field will be initialized as null. However I have
not verified this with a serialization test. If this new field is null
then the format will behave as if duplicates is set to DISALLOW.
I do not know the user case for serializing a CSVFormat. It would not
be within the library. A reference is kept in CSVParser and CSVPrinter
but these are not Serializable.
The serialization support dates back to the legacy codebase used to
create the project. Serialization can be replaced by other more
configurable libraries to read/write binary objects. Perhaps we just
need to add to the release notes that CSVFormat serialization is no
longer supported.
> [ERROR] java.method.exception.checkedRemoved: method java.util.List<org.apache.commons.csv.CSVRecord> org.apache.commons.csv.CSVParser::getRecords(): Method no longer throws checked exceptions: java.io.IOException. https://revapi.org/revapi-java/differences.html#java.method.exception.checkedRemoved
This is a source incompatible error. It is binary compatible.
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by sm...@outlook.de.
Hello
CSV-264 (Add DuplicateHeaderMode) introduces bugs that should be fixed before shipping 1.10.0 IMO
- missing default
- broken serialization of class CSVFormat
I raised these issues in CSV-302.
The serialization issue is caught by Revapi. I had suggested to include Revapi in the project (CSV-303) or alternatively add revapi and drop clirr in commons-parent but that more or less fell on deaf ears.
mvn org.revapi:revapi-maven-plugin:check
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 3.560 s
[INFO] Finished at: 2022-10-17T11:30:21+02:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.revapi:revapi-maven-plugin:0.14.7:check (default-cli) on project commons-csv: The following API problems caused the build to fail:
[ERROR] java.field.serialVersionUIDUnchanged: field org.apache.commons.csv.CSVFormat.serialVersionUID: The class changed in an incompatible way with regards to serialization but the serialVersionUID field stayed unchanged. This might be ok and/or desired but is suspicious. https://revapi.org/revapi-java/differences.html#java.field.serialVersionUIDUnchanged
[ERROR] java.method.exception.checkedRemoved: method java.util.List<org.apache.commons.csv.CSVRecord> org.apache.commons.csv.CSVParser::getRecords(): Method no longer throws checked exceptions: java.io.IOException. https://revapi.org/revapi-java/differences.html#java.method.exception.checkedRemoved
Also noticed that class DuplicateHeaderMode and method CSVFormat.getDuplicateHeaderMode() are incorrectly annotated with "@since 1.9.0". Both are new in version 1.10.0.
My vote is -1 assuming I am allowed to vote.
Kind regards,
Markus
From: Gary Gregory <ga...@gmail.com>
Sent: Sunday, October 16, 2022 14:48
To: Commons Developers List <de...@commons.apache.org>
Subject: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
To: dev@commons.apache.org
We have fixed a few bugs and added some enhancements since Apache
Commons CSV 1.9.0 was released, so I would like to release Apache
Commons CSV 1.10.0.
Apache Commons CSV 1.10.0 RC1 is available for review here:
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1 (svn
revision 57404)
The Git tag commons-csv-1.10.0-RC1 commit for this RC is
1bd1fd8e6065da9d07b5a3a1723b059246b14001 which you can browse here:
https://gitbox.apache.org/repos/asf?p=commons-csv.git;a=commit;h=1bd1fd8e6065da9d07b5a3a1723b059246b14001
You may checkout this tag using:
git clone https://gitbox.apache.org/repos/asf/commons-csv.git
--branch commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
Maven artifacts are here:
https://repository.apache.org/content/repositories/orgapachecommons-1600/org/apache/commons/commons-csv/1.10.0/
These are the artifacts and their hashes:
#Release SHA-512s
#Sun Oct 16 08:32:39 EDT 2022
Apache\ Commons\
CSV-1.10.0.spdx.rdf.xml=c8cf7495637ce5282c72aa8e8ef4b93f206fbbdf21c7b337a34daa3b89af9816073d0185118cab4cdbed5104cc7f090e81efb614594079b02829c96829fe8aeb
commons-csv-1.10.0-bin.tar.gz=5b0ff3f5cc4aeff6c2aa2c11e92ea8b1b1fbbcd098ba79d97bcfd9af3ea08a9ebc922e5237bda8abcb238eb0d2fe7a5ef27534d0d796a1fc77691684a698f3d1
commons-csv-1.10.0-bin.zip=a52caa6ccda5b830c133c965fd6843d8f960e1ce3ef83decf5435022481746093cf8b4308c1fded21a168b751724b4a9f5e72f3eca34381828968bff89e7239b
commons-csv-1.10.0-bom.json=0e3e6652e520afb697461a3f6b33458dc8ef1dd157c2b1442664b8f12f7504226916e64960cea5cc92dd2b923d18047ed921be60eb7febfbeffc8ac3e3254fc0
commons-csv-1.10.0-bom.xml=91e494b94eea35ee7d85c8d8dbdc5029f981e0c5a8b1aae2b0f3cd77d1cea0fb0fc8c857d18d41ca055d12c4c4eb1adf70582f178a88751b836861c3286283db
commons-csv-1.10.0-javadoc.jar=719656f5399e0889af2e8b40f49458d0dec6e8ae85413f36caa2187235c68d9e42502bf07c66c0bcc7c5c3427bc0aac51f9897581842d303501bbff5ec73bdc0
commons-csv-1.10.0-sources.jar=c78c8fa112ef35d6d6c061b05e08816ce516a3acd56d720c59dbdaeb9d1202b16294233bd2aa2fac91cfa759456f8d7e08951876d85decdf0b8fb215a8e31754
commons-csv-1.10.0-src.tar.gz=a0d3a7aedab567aaa32a3dd1ceddeff412e686f3fd2b0805993db7e3dd4804b4d5d59e70f1500f23e8d0ba6531832e9f20d0c298bd443d9825724d5d1b1c87b3
commons-csv-1.10.0-src.zip=5cbdbcf367c9dd78e76228817592d70378b3cd8d919fdcd7df5c3eb2d7ebd8bd47fbb17376a631efb640be4115d42d3cc7aafa866bfefdfe073d5a4fcf625e0b
commons-csv-1.10.0-test-sources.jar=0d77c791934609b176457df5c60fdd5bfa3c6f392375adef81e5e7cd31de7fb25bd91b0bbe60a9ff46acb62efc2a33660adff24850370b7546a71374aa267f94
commons-csv-1.10.0-tests.jar=9a08e4d2ec865d9e1dafc01e4ee58804490b2d50f886bac47a5235581077b31a15dda5b3f8f2adb5a70be8794cca886126d980d6d3722eec2e10471fc24e1940
I have tested this with 'mvn' and 'mvn -V -Duser.name=$my_apache_id
-Prelease -Ptest-deploy -P jacoco -P japicmp clean package site
deploy' using:
Darwin *** 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:17:10
PDT 2022; root:xnu-8020.140.49~2/RELEASE_X86_64 x86_64
Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)
Maven home: /usr/local/Cellar/maven/3.8.6/libexec
Java version: 1.8.0_345, vendor: Homebrew, runtime:
/usr/local/Cellar/openjdk@8/1.8.0+345/libexec/openjdk.jdk/Contents/Home/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "12.6", arch: "x86_64", family: "mac"
Details of changes since 1.9.0 are in the release notes:
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/RELEASE-NOTES.txt
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/changes-report.html
Site:
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/index.html
(note some *relative* links are broken and the 1.10.0 directories
are not yet created - these will be OK once the site is deployed.)
JApiCmp Report (compared to 1.9.0):
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/japicmp.html
RAT Report:
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/rat-report.html
KEYS:
https://downloads.apache.org/commons/KEYS
Please review the release candidate and vote.
This vote will close no sooner than 72 hours from now.
[ ] +1 Release these artifacts
[ ] +0 OK, but...
[ ] -0 OK, but really should fix...
[ ] -1 I oppose this release because...
Thank you,
Gary Gregory,
Release Manager (using key 86fdc7e2a11262cb)
For following is intended as a helper and refresher for reviewers.
Validating a release candidate
==============================
These guidelines are NOT complete.
Requirements: Git, Java, Maven.
You can validate a release from a release candidate (RC) tag as follows.
1) Clone and checkout the RC tag
git clone https://gitbox.apache.org/repos/asf/commons-csv.git --branch
commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
cd commons-csv-1.10.0-RC1
2) Check Apache licenses
This step is not required if the site includes a RAT report page which
you then must check.
mvn apache-rat:check
3) Check binary compatibility
Use JApiCmp with the japicmp Maven Profile:
This step is not required if the site includes a JApiCmp report page
which you then must check.
mvn install -DskipTests -P japicmp japicmp:cmp
4) Build the package
mvn -V clean package
You can record the Maven and Java version produced by -V in your VOTE reply.
To gather OS information from a command line:
Windows: ver
Linux: uname -a
5) Build the site for a single module project
Note: Some plugins require the components to be installed instead of packaged.
mvn site
Check the site reports in:
- Windows: target\site\index.html
- Linux: target/site/index.html
-the end-
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Gary Gregory <ga...@gmail.com>.
I will update all of this now but not cut another RC, as you said, none of
these are blockers. I'm doing this in the release branch for convenience.
Tracking below..
On Sun, Oct 16, 2022 at 10:57 AM Alex Herbert <al...@gmail.com>
wrote:
>
> Verified sha512 and asc signatures on the binary and source distributions.
>
> Built the src.tar.gz using:
>
> mvn clean install site -Pjacoco,japicmp
>
> with:
>
> Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
> Maven home: /usr/local/apache-maven-3.6.3
> Java version: 17, vendor: Oracle Corporation, runtime:
> /Library/Java/JavaVirtualMachines/jdk-17.jdk/Contents/Home
> Default locale: en_GB, platform encoding: UTF-8
> OS name: "mac os x", version: "12.6", arch: "x86_64", family: "mac"
>
> Built from the git tag using:
>
> mvn install site
>
> with:
>
> Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
> Maven home: /usr/local/apache-maven-3.6.3
> Java version: 1.8.0_301, vendor: Oracle Corporation, runtime:
> /Library/Java/JavaVirtualMachines/jdk1.8.0_301.jdk/Contents/Home/jre
> Default locale: en_US, platform encoding: UTF-8
> OS name: "mac os x", version: "10.16", arch: "x86_64", family: "mac"
>
> Note: PMD: The build has warnings about deprecated rules to be removed in
> 7.0. These should be cleaned up.
I removed the deprecated rules.
>
> Site:
>
> The main index.html page has the dependency example using version 1.9.0.
> This should be 1.10.0. Given this is in the dependency information section
> under project information then perhaps it could be dropped to avoid
> maintenance.
Agreed & done.
>
> The CI management section is wrong (refers to commons-parent). This is
> picked up from the <ciManagement> element in the POM of commons-parent.
The
> element should be overridden in the commons-csv POM.
Done
>
> Surefire report:
>
> CSV-213 test is disabled. This is marked as fixed in CSV 1.4 in the Jira
> ticket. Perhaps it can be reenabled?
Yep & done.
There are a lot of other disabled
> tests too (29 occurrences found in the report). I did not check them all.
> This should be looked at in the future as they may actually pass and were
> not reenabled after the fix.
I was enabled to enable some CSVPrinterTest tests
* testJira135_part1
* testJira135_part3
* testRandomPostgreSqlText
The disabled PostgreSQL tests just need to be reworked to account for the
bug fix in this release.
The other disabled test either need research or underlying issues fixed.
>
> CSVFormat.printRecord(final Appendable appendable, final Object... values)
> has no coverage.
Fixed.
Perhaps this is related to the new
> CSVPrinter.printRecord(Stream) method (e.g. execution paths have switched
> to use the new method).
>
> JApiCmp - All changes OK. I checked the @since tags for the new API:
>
> DuplicateHeaderMode is a new class. The since tag is for 1.9.0. This
should
> be 1.10.0.
Fixed
>
> CSVFormat.getDuplicateHeaderMode @since tag is also 1.9.0.
Fixed
>
> new method: org.apache.commons.csv.CSVFormat$Builder
setDuplicateHeaderMode
> has no @since tag
>
> All other new API @since tags are OK.
>
> changes, rat, jira, javadoc, Checkstyle, spotbugs, PMD all OK.
>
>
> Overall no major issues, just some minor issues to clean up but I do not
> think these are blockers.
>
> +1: Release these artifacts
Thank you for the thorough review!
>
> Alex
>
>
>
>
> On Sun, 16 Oct 2022 at 13:49, Gary Gregory <ga...@gmail.com> wrote:
>
> > To: dev@commons.apache.org
> > We have fixed a few bugs and added some enhancements since Apache
> > Commons CSV 1.9.0 was released, so I would like to release Apache
> > Commons CSV 1.10.0.
> >
> > Apache Commons CSV 1.10.0 RC1 is available for review here:
> > https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1 (svn
> > revision 57404)
> >
> > The Git tag commons-csv-1.10.0-RC1 commit for this RC is
> > 1bd1fd8e6065da9d07b5a3a1723b059246b14001 which you can browse here:
> >
> >
https://gitbox.apache.org/repos/asf?p=commons-csv.git;a=commit;h=1bd1fd8e6065da9d07b5a3a1723b059246b14001
> > You may checkout this tag using:
> > git clone https://gitbox.apache.org/repos/asf/commons-csv.git
> > --branch <https://gitbox.apache.org/repos/asf/commons-csv.git--branch>
> > commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
> >
> > Maven artifacts are here:
> >
> >
https://repository.apache.org/content/repositories/orgapachecommons-1600/org/apache/commons/commons-csv/1.10.0/
> >
> > These are the artifacts and their hashes:
> >
> > #Release SHA-512s
> > #Sun Oct 16 08:32:39 EDT 2022
> > Apache\ Commons\
> >
> >
CSV-1.10.0.spdx.rdf.xml=c8cf7495637ce5282c72aa8e8ef4b93f206fbbdf21c7b337a34daa3b89af9816073d0185118cab4cdbed5104cc7f090e81efb614594079b02829c96829fe8aeb
> >
> >
commons-csv-1.10.0-bin.tar.gz=5b0ff3f5cc4aeff6c2aa2c11e92ea8b1b1fbbcd098ba79d97bcfd9af3ea08a9ebc922e5237bda8abcb238eb0d2fe7a5ef27534d0d796a1fc77691684a698f3d1
> >
> >
commons-csv-1.10.0-bin.zip=a52caa6ccda5b830c133c965fd6843d8f960e1ce3ef83decf5435022481746093cf8b4308c1fded21a168b751724b4a9f5e72f3eca34381828968bff89e7239b
> >
> >
commons-csv-1.10.0-bom.json=0e3e6652e520afb697461a3f6b33458dc8ef1dd157c2b1442664b8f12f7504226916e64960cea5cc92dd2b923d18047ed921be60eb7febfbeffc8ac3e3254fc0
> >
> >
commons-csv-1.10.0-bom.xml=91e494b94eea35ee7d85c8d8dbdc5029f981e0c5a8b1aae2b0f3cd77d1cea0fb0fc8c857d18d41ca055d12c4c4eb1adf70582f178a88751b836861c3286283db
> >
> >
commons-csv-1.10.0-javadoc.jar=719656f5399e0889af2e8b40f49458d0dec6e8ae85413f36caa2187235c68d9e42502bf07c66c0bcc7c5c3427bc0aac51f9897581842d303501bbff5ec73bdc0
> >
> >
commons-csv-1.10.0-sources.jar=c78c8fa112ef35d6d6c061b05e08816ce516a3acd56d720c59dbdaeb9d1202b16294233bd2aa2fac91cfa759456f8d7e08951876d85decdf0b8fb215a8e31754
> >
> >
commons-csv-1.10.0-src.tar.gz=a0d3a7aedab567aaa32a3dd1ceddeff412e686f3fd2b0805993db7e3dd4804b4d5d59e70f1500f23e8d0ba6531832e9f20d0c298bd443d9825724d5d1b1c87b3
> >
> >
commons-csv-1.10.0-src.zip=5cbdbcf367c9dd78e76228817592d70378b3cd8d919fdcd7df5c3eb2d7ebd8bd47fbb17376a631efb640be4115d42d3cc7aafa866bfefdfe073d5a4fcf625e0b
> >
> >
commons-csv-1.10.0-test-sources.jar=0d77c791934609b176457df5c60fdd5bfa3c6f392375adef81e5e7cd31de7fb25bd91b0bbe60a9ff46acb62efc2a33660adff24850370b7546a71374aa267f94
> >
> >
commons-csv-1.10.0-tests.jar=9a08e4d2ec865d9e1dafc01e4ee58804490b2d50f886bac47a5235581077b31a15dda5b3f8f2adb5a70be8794cca886126d980d6d3722eec2e10471fc24e1940
> >
> > I have tested this with 'mvn' and 'mvn -V -Duser.name=$my_apache_id
> > -Prelease -Ptest-deploy -P jacoco -P japicmp clean package site
> > deploy' using:
> >
> > Darwin *** 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:17:10
> > PDT 2022; root:xnu-8020.140.49~2/RELEASE_X86_64 x86_64
> >
> > Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)
> > Maven home: /usr/local/Cellar/maven/3.8.6/libexec
> > Java version: 1.8.0_345, vendor: Homebrew, runtime:
> > /usr/local/Cellar/openjdk@8
> > /1.8.0+345/libexec/openjdk.jdk/Contents/Home/jre
> > Default locale: en_US, platform encoding: UTF-8
> > OS name: "mac os x", version: "12.6", arch: "x86_64", family: "mac"
> >
> > Details of changes since 1.9.0 are in the release notes:
> >
> >
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/RELEASE-NOTES.txt
> >
> >
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/changes-report.html
> >
> > Site:
> >
> >
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/index.html
> > (note some *relative* links are broken and the 1.10.0 directories
> > are not yet created - these will be OK once the site is deployed.)
> >
> > JApiCmp Report (compared to 1.9.0):
> >
> >
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/japicmp.html
> >
> > RAT Report:
> >
> >
https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/rat-report.html
> >
> > KEYS:
> > https://downloads.apache.org/commons/KEYS
> >
> > Please review the release candidate and vote.
> > This vote will close no sooner than 72 hours from now.
> >
> > [ ] +1 Release these artifacts
> > [ ] +0 OK, but...
> > [ ] -0 OK, but really should fix...
> > [ ] -1 I oppose this release because...
> >
> > Thank you,
> >
> > Gary Gregory,
> > Release Manager (using key 86fdc7e2a11262cb)
> >
> > For following is intended as a helper and refresher for reviewers.
> >
> > Validating a release candidate
> > ==============================
> >
> > These guidelines are NOT complete.
> >
> > Requirements: Git, Java, Maven.
> >
> > You can validate a release from a release candidate (RC) tag as follows.
> >
> > 1) Clone and checkout the RC tag
> >
> > git clone https://gitbox.apache.org/repos/asf/commons-csv.git --branch
> > commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
> > cd commons-csv-1.10.0-RC1
> >
> > 2) Check Apache licenses
> >
> > This step is not required if the site includes a RAT report page which
> > you then must check.
> >
> > mvn apache-rat:check
> >
> > 3) Check binary compatibility
> >
> > Use JApiCmp with the japicmp Maven Profile:
> >
> > This step is not required if the site includes a JApiCmp report page
> > which you then must check.
> >
> > mvn install -DskipTests -P japicmp japicmp:cmp
> >
> > 4) Build the package
> >
> > mvn -V clean package
> >
> > You can record the Maven and Java version produced by -V in your VOTE
> > reply.
> > To gather OS information from a command line:
> > Windows: ver
> > Linux: uname -a
> >
> > 5) Build the site for a single module project
> >
> > Note: Some plugins require the components to be installed instead of
> > packaged.
> >
> > mvn site
> > Check the site reports in:
> > - Windows: target\site\index.html
> > - Linux: target/site/index.html
> >
> > -the end-
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
Re: [VOTE] Release Apache Commons CSV 1.10.0 based on RC1
Posted by Alex Herbert <al...@gmail.com>.
Verified sha512 and asc signatures on the binary and source distributions.
Built the src.tar.gz using:
mvn clean install site -Pjacoco,japicmp
with:
Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
Maven home: /usr/local/apache-maven-3.6.3
Java version: 17, vendor: Oracle Corporation, runtime:
/Library/Java/JavaVirtualMachines/jdk-17.jdk/Contents/Home
Default locale: en_GB, platform encoding: UTF-8
OS name: "mac os x", version: "12.6", arch: "x86_64", family: "mac"
Built from the git tag using:
mvn install site
with:
Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
Maven home: /usr/local/apache-maven-3.6.3
Java version: 1.8.0_301, vendor: Oracle Corporation, runtime:
/Library/Java/JavaVirtualMachines/jdk1.8.0_301.jdk/Contents/Home/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "10.16", arch: "x86_64", family: "mac"
Note: PMD: The build has warnings about deprecated rules to be removed in
7.0. These should be cleaned up.
Site:
The main index.html page has the dependency example using version 1.9.0.
This should be 1.10.0. Given this is in the dependency information section
under project information then perhaps it could be dropped to avoid
maintenance.
The CI management section is wrong (refers to commons-parent). This is
picked up from the <ciManagement> element in the POM of commons-parent. The
element should be overridden in the commons-csv POM.
Surefire report:
CSV-213 test is disabled. This is marked as fixed in CSV 1.4 in the Jira
ticket. Perhaps it can be reenabled? There are a lot of other disabled
tests too (29 occurrences found in the report). I did not check them all.
This should be looked at in the future as they may actually pass and were
not reenabled after the fix.
CSVFormat.printRecord(final Appendable appendable, final Object... values)
has no coverage. Perhaps this is related to the new
CSVPrinter.printRecord(Stream) method (e.g. execution paths have switched
to use the new method).
JApiCmp - All changes OK. I checked the @since tags for the new API:
DuplicateHeaderMode is a new class. The since tag is for 1.9.0. This should
be 1.10.0.
CSVFormat.getDuplicateHeaderMode @since tag is also 1.9.0.
new method: org.apache.commons.csv.CSVFormat$Builder setDuplicateHeaderMode
has no @since tag
All other new API @since tags are OK.
changes, rat, jira, javadoc, Checkstyle, spotbugs, PMD all OK.
Overall no major issues, just some minor issues to clean up but I do not
think these are blockers.
+1: Release these artifacts
Alex
On Sun, 16 Oct 2022 at 13:49, Gary Gregory <ga...@gmail.com> wrote:
> To: dev@commons.apache.org
> We have fixed a few bugs and added some enhancements since Apache
> Commons CSV 1.9.0 was released, so I would like to release Apache
> Commons CSV 1.10.0.
>
> Apache Commons CSV 1.10.0 RC1 is available for review here:
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1 (svn
> revision 57404)
>
> The Git tag commons-csv-1.10.0-RC1 commit for this RC is
> 1bd1fd8e6065da9d07b5a3a1723b059246b14001 which you can browse here:
>
> https://gitbox.apache.org/repos/asf?p=commons-csv.git;a=commit;h=1bd1fd8e6065da9d07b5a3a1723b059246b14001
> You may checkout this tag using:
> git clone https://gitbox.apache.org/repos/asf/commons-csv.git
> --branch <https://gitbox.apache.org/repos/asf/commons-csv.git--branch>
> commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
>
> Maven artifacts are here:
>
> https://repository.apache.org/content/repositories/orgapachecommons-1600/org/apache/commons/commons-csv/1.10.0/
>
> These are the artifacts and their hashes:
>
> #Release SHA-512s
> #Sun Oct 16 08:32:39 EDT 2022
> Apache\ Commons\
>
> CSV-1.10.0.spdx.rdf.xml=c8cf7495637ce5282c72aa8e8ef4b93f206fbbdf21c7b337a34daa3b89af9816073d0185118cab4cdbed5104cc7f090e81efb614594079b02829c96829fe8aeb
>
> commons-csv-1.10.0-bin.tar.gz=5b0ff3f5cc4aeff6c2aa2c11e92ea8b1b1fbbcd098ba79d97bcfd9af3ea08a9ebc922e5237bda8abcb238eb0d2fe7a5ef27534d0d796a1fc77691684a698f3d1
>
> commons-csv-1.10.0-bin.zip=a52caa6ccda5b830c133c965fd6843d8f960e1ce3ef83decf5435022481746093cf8b4308c1fded21a168b751724b4a9f5e72f3eca34381828968bff89e7239b
>
> commons-csv-1.10.0-bom.json=0e3e6652e520afb697461a3f6b33458dc8ef1dd157c2b1442664b8f12f7504226916e64960cea5cc92dd2b923d18047ed921be60eb7febfbeffc8ac3e3254fc0
>
> commons-csv-1.10.0-bom.xml=91e494b94eea35ee7d85c8d8dbdc5029f981e0c5a8b1aae2b0f3cd77d1cea0fb0fc8c857d18d41ca055d12c4c4eb1adf70582f178a88751b836861c3286283db
>
> commons-csv-1.10.0-javadoc.jar=719656f5399e0889af2e8b40f49458d0dec6e8ae85413f36caa2187235c68d9e42502bf07c66c0bcc7c5c3427bc0aac51f9897581842d303501bbff5ec73bdc0
>
> commons-csv-1.10.0-sources.jar=c78c8fa112ef35d6d6c061b05e08816ce516a3acd56d720c59dbdaeb9d1202b16294233bd2aa2fac91cfa759456f8d7e08951876d85decdf0b8fb215a8e31754
>
> commons-csv-1.10.0-src.tar.gz=a0d3a7aedab567aaa32a3dd1ceddeff412e686f3fd2b0805993db7e3dd4804b4d5d59e70f1500f23e8d0ba6531832e9f20d0c298bd443d9825724d5d1b1c87b3
>
> commons-csv-1.10.0-src.zip=5cbdbcf367c9dd78e76228817592d70378b3cd8d919fdcd7df5c3eb2d7ebd8bd47fbb17376a631efb640be4115d42d3cc7aafa866bfefdfe073d5a4fcf625e0b
>
> commons-csv-1.10.0-test-sources.jar=0d77c791934609b176457df5c60fdd5bfa3c6f392375adef81e5e7cd31de7fb25bd91b0bbe60a9ff46acb62efc2a33660adff24850370b7546a71374aa267f94
>
> commons-csv-1.10.0-tests.jar=9a08e4d2ec865d9e1dafc01e4ee58804490b2d50f886bac47a5235581077b31a15dda5b3f8f2adb5a70be8794cca886126d980d6d3722eec2e10471fc24e1940
>
> I have tested this with 'mvn' and 'mvn -V -Duser.name=$my_apache_id
> -Prelease -Ptest-deploy -P jacoco -P japicmp clean package site
> deploy' using:
>
> Darwin *** 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:17:10
> PDT 2022; root:xnu-8020.140.49~2/RELEASE_X86_64 x86_64
>
> Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)
> Maven home: /usr/local/Cellar/maven/3.8.6/libexec
> Java version: 1.8.0_345, vendor: Homebrew, runtime:
> /usr/local/Cellar/openjdk@8
> /1.8.0+345/libexec/openjdk.jdk/Contents/Home/jre
> Default locale: en_US, platform encoding: UTF-8
> OS name: "mac os x", version: "12.6", arch: "x86_64", family: "mac"
>
> Details of changes since 1.9.0 are in the release notes:
>
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/RELEASE-NOTES.txt
>
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/changes-report.html
>
> Site:
>
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/index.html
> (note some *relative* links are broken and the 1.10.0 directories
> are not yet created - these will be OK once the site is deployed.)
>
> JApiCmp Report (compared to 1.9.0):
>
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/japicmp.html
>
> RAT Report:
>
> https://dist.apache.org/repos/dist/dev/commons/csv/1.10.0-RC1/site/rat-report.html
>
> KEYS:
> https://downloads.apache.org/commons/KEYS
>
> Please review the release candidate and vote.
> This vote will close no sooner than 72 hours from now.
>
> [ ] +1 Release these artifacts
> [ ] +0 OK, but...
> [ ] -0 OK, but really should fix...
> [ ] -1 I oppose this release because...
>
> Thank you,
>
> Gary Gregory,
> Release Manager (using key 86fdc7e2a11262cb)
>
> For following is intended as a helper and refresher for reviewers.
>
> Validating a release candidate
> ==============================
>
> These guidelines are NOT complete.
>
> Requirements: Git, Java, Maven.
>
> You can validate a release from a release candidate (RC) tag as follows.
>
> 1) Clone and checkout the RC tag
>
> git clone https://gitbox.apache.org/repos/asf/commons-csv.git --branch
> commons-csv-1.10.0-RC1 commons-csv-1.10.0-RC1
> cd commons-csv-1.10.0-RC1
>
> 2) Check Apache licenses
>
> This step is not required if the site includes a RAT report page which
> you then must check.
>
> mvn apache-rat:check
>
> 3) Check binary compatibility
>
> Use JApiCmp with the japicmp Maven Profile:
>
> This step is not required if the site includes a JApiCmp report page
> which you then must check.
>
> mvn install -DskipTests -P japicmp japicmp:cmp
>
> 4) Build the package
>
> mvn -V clean package
>
> You can record the Maven and Java version produced by -V in your VOTE
> reply.
> To gather OS information from a command line:
> Windows: ver
> Linux: uname -a
>
> 5) Build the site for a single module project
>
> Note: Some plugins require the components to be installed instead of
> packaged.
>
> mvn site
> Check the site reports in:
> - Windows: target\site\index.html
> - Linux: target/site/index.html
>
> -the end-
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>