Skip to content

Improve accuracy of supports_string_literal_concatenation_with_newline#2348

Merged
yoavcloud merged 3 commits into
apache:mainfrom
yoavcloud:cocnat_newline_comment
May 28, 2026
Merged

Improve accuracy of supports_string_literal_concatenation_with_newline#2348
yoavcloud merged 3 commits into
apache:mainfrom
yoavcloud:cocnat_newline_comment

Conversation

@yoavcloud
Copy link
Copy Markdown
Contributor

No description provided.

@yoavcloud yoavcloud requested a review from iffyio May 21, 2026 16:41
Comment thread src/parser/mod.rs Outdated
// Tokenizer includes the newline in the single line comment
// so we need to check for it specifically here, otherwise the newline will
// not be consumed as a separate token.
Token::Whitespace(Whitespace::SingleLineComment { comment, .. }) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm wondering would it make sense to strip out the newline from the tokenizer instead? thinking that feels more like expected behavior for folks that rely on the content of a single line comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It would, I just thought it would be a disruptive change. However, I gave it a try and I think it makes sense. Take a look.

@yoavcloud yoavcloud force-pushed the cocnat_newline_comment branch from 6f3bbaa to 6416bc7 Compare May 28, 2026 09:15
Copy link
Copy Markdown
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @yoavcloud!

@yoavcloud yoavcloud force-pushed the cocnat_newline_comment branch from 9dd202c to 9e80552 Compare May 28, 2026 13:11
@yoavcloud yoavcloud enabled auto-merge May 28, 2026 13:18
@yoavcloud yoavcloud added this pull request to the merge queue May 28, 2026
Merged via the queue into apache:main with commit 0d63896 May 28, 2026
10 checks passed
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