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

Doc changes to avoid user confusion of seperating tags with , in expr… #911

Conversation

shivam-sehgal
Copy link

…ession

🤔 What's changed?

I came across cucumber/cucumber-jvm#2776. Although the documentation specifies the expected format for tags, it seems that the pattern used by the user, 'https://github.com/Step1,@step2,' did not generate any error. This discrepancy has the potential to confuse users. To address this, it would be beneficial to add this message to doc

more discussion on this could be found in this PR

⚡️ What's your motivation?

heavily use this library, and wanted to contribute.

If it's related to another issue or bugfix, add it as a reference here,
e.g. "Fixes cucumber/cucumber-js#99"
-->
cucumber/cucumber-jvm#2776
cucumber/tag-expressions#107

📋 Checklist:

Copy link
Contributor

@luke-hill luke-hill left a comment

Choose a reason for hiding this comment

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

Will reject this as it's using legacy information

@@ -969,6 +969,8 @@ A tag expression is an *infix boolean expression*. Below are some examples:
| `@smoke and @fast` | Scenarios tagged with both `@smoke` and `@fast` |
| `@gui or @database` | Scenarios tagged with either `@gui` or `@database` |

Note: Using a `,` to separate tags in expressions such as `@tag1,@tag2` will cause the entire expression to be parsed as a single tag, potentially leading to unintended outcomes. It's required to separate the tags with space or operators like `and` `or` and `not`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is legacy and we shouldn't be documenting this. Please use the documentation above.

Copy link
Contributor

@mpkorstanje mpkorstanje Aug 15, 2023

Choose a reason for hiding this comment

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

@luke-hill I think the purpose was to explain people that there was a different syntax for tag expressions in the past and that they should upgrade theirs.

@shivam-sehgal this change is missing quite some context to help understand the reader what is going on. Consider a section along the line of "Upgrading from legacy tag expressions" that explains what old tag expressions may have looked like and how they should be rewritten.

Copy link
Contributor

Choose a reason for hiding this comment

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

In previous areas we've actively removed legacy information, so if we're going down that route (removing legacy info), I'd rather keep consistent than not.

But yes notwithstanding that, if something indicated that this isn't advised then yes I figure that could be ok

@luke-hill luke-hill closed this Aug 15, 2023
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.

3 participants