Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] Fix comment parsing to support multiple comments (backported) #671

Merged

Conversation

ziegenberg
Copy link
Contributor

Because of an eager consumption of whitespace, the rule parsing would swallow a trailing comment, meaning the comment for the next rule would be affected. This patch addresses this by only consuming real whitespace without comments after a rule.

Fixes #173

…s#671)

Because of an eager consumption of whitespace, the rule parsing would
swallow a trailing comment, meaning the comment for the next rule would
be affected. This patch addresses this by only consuming real whitespace
without comments after a rule.

Fixes MyIntervals#173

Signed-off-by: Daniel Ziegenberg <[email protected]>
@ziegenberg ziegenberg force-pushed the support-multiple-comments-backported branch from 2b15ee9 to 6ccfb21 Compare August 25, 2024 17:32
@oliverklee oliverklee added this to the 8.6.1 or 8.7.0 milestone Aug 25, 2024
@oliverklee oliverklee added the bug label Aug 25, 2024
Copy link
Contributor

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

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

Thanks!

@oliverklee oliverklee merged commit ddd9ea7 into MyIntervals:v8.x Aug 25, 2024
19 checks passed
@oliverklee
Copy link
Contributor

I'm a bit confused … do I need to forward-port this, or is this not necessary due to other upcoming changes in main?

(In general, if we want to create a PR that we are also planning to backport, we first create the PR for main and then backport it to the maintenance branch after it has been merged.)

@oliverklee
Copy link
Contributor

I've now forward-ported this in #672. For future patches, please let's create the change for main first, and I (or another maintainer) will then take care of the backport. (This also works better with the branch protection in place, where we need another reviewer if we want to merge to main and would need to override this protection if we want to do a forward-port without an additional review.)

@ziegenberg ziegenberg deleted the support-multiple-comments-backported branch August 26, 2024 17:07
@ziegenberg
Copy link
Contributor Author

I wasn't done with the patches. As the solution for v8.x looks very different from the proposed solution for v9.0, I was about to create different patches. Either back-porting or forward-porting would have meant a completely different approach.

@oliverklee
Copy link
Contributor

All right. I’m looking forward to your PRs! 🙏

@oliverklee
Copy link
Contributor

And if you think talking things through would be helpful, please feel free to chat me up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants