-
-
Notifications
You must be signed in to change notification settings - Fork 694
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
tag-expressions: support escape backslashes in tag expressions #1778
tag-expressions: support escape backslashes in tag expressions #1778
Conversation
Thanks a lot @yusuke-noda for your PR! 😀 We may have to move it to https://github.com/cucumber/tag-expressions 🤔 |
escaped = false | ||
} else { | ||
escaped = true | ||
} |
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.
Structurally the go implementation differs quite a bit from the other implementations. So I would not be opposed to lifting this case out into a separate if-block so that the escaped = false
in the unicode.IsSpace(c) {
block is not needed and so that the different implementations resemble each other again.
@aurelien-reeves a "maybe" isn't actionable. Can you either provide clear directions or indicate that you are still looking for a solution? |
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.
In principle the changes look good to me.
@yusuke-noda are you participating in hacktober fest? I don't think we can merge this straight away but I can mark the PR as accepted for the purposes of the event.
That was the point of the "🤔" smiley. To start discussing about that because I am not sure of what should be done. I think we should move that PR to https://github.com/cucumber/tag-expressions |
@mattwynne is there a technical reason that tag-expressions is still in located in common? Did the migration to https://github.com/cucumber/tag-expressions fail? |
Thank you, but i am not participating in hacktober fest. |
It didn't fail, we're just not done with it. I've had to focus on other things this week. We still need to get the CI working over there, which might make it a tricky environment to merge a PR into, but I'm fairly ambivalent about which repo we merge it into for now, as long as we don't forget to migrate the commits to the new repo at some point. |
For now I suggest to continue working here, on the monorepo. I have opened an issue on the new repo to avoid forgetting about it later: cucumber/tag-expressions#4 |
Let's merge this here. Then make a release. When that's done there won't be any open issues/PRs for tag-expressions, and we can do the migration to the new repo again. |
For whomever merges this - please update the CHANGELOG.md |
The escaping backslash was added to the tag and if the escaped character was a space, it was skipped even though being escaped. Re #1778.
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.
From my reading, the changes are correct. it looks like we can merge this PR as it is (and make the modifications that @mpkorstanje desires when this has its own repository).
…noda/common into tag-expressions-escape-backslash
Hi @yusuke-noda, 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, |
) The escaping backslash was added to the tag and if the escaped character was a space, it was skipped even though being escaped. Re #1778.
Summary
Add support "\\" escape for backslash in tag expressions.
Details
Javascript, Java, and Go implementations of Tag Expressions cannnot handle tags containing backslashes, but Tag Expressions should be able to handle tags containing backslashes.
Motivation and Context
Fixes #1776
Fixes #1777
How Has This Been Tested?
Added the following test cases:
a\\ and b
\a and b
a\ and b
a and b\
( a and b\\)
a\\\( and b\\\)
(a and \b)
Screenshots (if appropriate):
Types of changes
Checklist: