Skip to content

Quote value starting with comment marker in minimal quote mode#610

Merged
garydgregory merged 2 commits into
apache:masterfrom
rootvector2:quote-comment-marker-first-char
Jun 16, 2026
Merged

Quote value starting with comment marker in minimal quote mode#610
garydgregory merged 2 commits into
apache:masterfrom
rootvector2:quote-comment-marker-first-char

Conversation

@rootvector2

Copy link
Copy Markdown
Contributor

printWithQuotes (the default MINIMAL quote mode) decides whether to encapsulate a value by special-casing only the hard-coded # (Constants.COMMENT) as the first character, so a configured comment marker greater than # such as ; is left unquoted. I hit this round-tripping printer output back through the parser: CSVFormat.DEFAULT.builder().setCommentMarker(';').get() prints ";id", "name" as ;id,name, and re-parsing with that same format reads the line as a comment and silently drops the whole record.

The encapsulation check is the right place to fix it, since that is already where the printer protects the default comment char, the delimiter, the quote and newlines. The change quotes a value whenever its first character equals the configured comment marker, so the output reads back as data.

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @rootvector2

Thank you for the PR.

Would you please update the new test to make sure that comments are properly printed. We want to make sure the new test shows the difference between printing a comment and printing a value that happens to start with the comment char.

Can you think of other cases to test?

@garydgregory garydgregory changed the title quote value starting with comment marker in minimal quote mode Quote value starting with comment marker in minimal quote mode Jun 15, 2026
@rootvector2

Copy link
Copy Markdown
Contributor Author

Done. The test now prints a real comment with printComment first, then the leading-marker value, so the output makes the difference explicit: ; a real comment stays a comment line while ;comment-like becomes ";comment-like".

Added a couple more cases too: the marker past the first character (a;b) is left unquoted since it can't start a comment, and a leading marker in a non-first column (;c) is still quoted. The round-trip check confirms the comment is dropped on read and both data records survive.

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rootvector2
Please don't bother pushing to github until you've run mvn by itself to run and fix all build failures 🦺

@rootvector2 rootvector2 force-pushed the quote-comment-marker-first-char branch from 4718744 to 3c2291c Compare June 16, 2026 17:16
@rootvector2

Copy link
Copy Markdown
Contributor Author

My fault. It was checkstyle OperatorWrap on the multi-line assertEquals concatenation, the + has to sit at the end of the line, not the start of the next. Fixed it and re-ran mvn by itself, build's green now with 0 violations. Amended into the test commit.

@garydgregory garydgregory merged commit 525dca2 into apache:master Jun 16, 2026
16 checks passed
garydgregory added a commit that referenced this pull request Jun 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants