-
Notifications
You must be signed in to change notification settings - Fork 133
Fix regression on optional regex submatches #221
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
Fix regression on optional regex submatches #221
Conversation
I have no clue about whether I should make any changes to the documentation (including CHANGELOG.md), so I left the 2 check boxes un-ticked for now. |
This regression was introduced in commit a7ff906 (Regex: use iterators for match results).
318a002
to
34345b3
Compare
Following the recent merges of #216 and #215, I've rebased and force-pushed my branch. |
This regression was introduced in commit a7ff906 (Regex: use iterators for match results).
34345b3
to
e7b8f64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR, this looks good!
@muggenhor: thank you for the review! Can you take care of merging this PR, or should we wait for @paoloambrosio to also have a look? |
I'll |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm late to the party, but top quality PR! 👍
Hi @ala-ableton, Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾 In return for this generous offer we hope you will:
On behalf of the Cucumber core team, |
Summary
Fixes a regression that was introduced in commit a7ff906 (Regex: use iterators for match results).
Details
Until commit a7ff906, the vector returned by
RegexMatch::getSubmatches()
contained an empty string item for each optional submatch that didn't match. Commit a7ff906 broke that behavior by skipping any submatch that didn't match.How Has This Been Tested?
I added a test (
matchesRegexWithOptionalSubmatches
) totests/unit/RegexTest.cpp
.I cherry-picked this test on top of 4dec371 (the parent commit of a7ff906) and verified that it was passing. I then verified that this test doesn't pass on top of the latest
master
without the corresponding change insrc/Regex.cpp
.Types of changes
Checklist: