-
Notifications
You must be signed in to change notification settings - Fork 92
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 parsing #215
Tag parsing #215
Conversation
src/Behat/Gherkin/Parser.php
Outdated
{ | ||
foreach ($tags as $tag) { | ||
if (preg_match('/\s/', $tag)) { | ||
throw new ParserException( |
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.
This could be E_USER_DEPRECATED
if we think whitespace tags are a valid current usage rather than an error
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.
What do you think about this @stof?
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.
do tag expressions in TagFilter support tags with whitespace in them or no ? If such tags are not usable in TagFilter, then I would go for a ParserException
I would vote for adding such API to consume the line only partially instead of the hack about |
@stof Yeah I will add something like |
@ciaranmcnulty I suggest confirming upstream whether comments are meant to be allowed at the end of any line. If the answer is yes, it might be a matter of changing |
It looks like they're only allowed at the end of tags lines, added in cucumber/common#880 |
2f5c867
to
bfc5d35
Compare
TagFilter is quite happy to be constructed with, and filter by tags with whitespace so there's a chance there are people depending on this functionality. trigger_error with a deprecation warning? |
@ciaranmcnulty then a deprecation is indeed better (and probably also in TagFilter if the filter uses a tag with whitespace) |
I think #220 is necessary to test the deprecations properly |
8b34eaa
to
1293188
Compare
@stof would appreciate feedback on how the deprecation is implemented |
When using that where ? |
On suite configuration like here: https://github.com/Sylius/Sylius/blob/master/src/Sylius/Behat/Resources/config/suites/ui/account/address_book.yml |
It's about |
I agree. The deprecation warning should allow for spaces around the operators. However, I would not do that in |
Since Gherkin 4.9.0 tags with whitespace are deprecated: use a comma as an explicit OR operator. See - Behat/Gherkin#213 - Behat/Gherkin#215 - https://behat.org/en/latest/user_guide/context/hooks.html#tagged-hooks Closes jhedstrom#600
Since Gherkin 4.9.0 tags with whitespace are deprecated: use a comma as an explicit OR operator. See - Behat/Gherkin#213 - Behat/Gherkin#215 - https://behat.org/en/latest/user_guide/context/hooks.html#tagged-hooks Closes jhedstrom#600
Since Gherkin 4.9.0 tags with whitespace are deprecated: use a comma as an explicit OR operator. See - Behat/Gherkin#213 - Behat/Gherkin#215 - https://behat.org/en/latest/user_guide/context/hooks.html#tagged-hooks Closes jhedstrom#600
Closes #210 and #213
Not too happy with the implementation yet
For the tag whitespace we could optionally show a warning rather than an exception for backwards compatibility