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

Unit test for CLI flag --name + doc update #1445

Merged
merged 7 commits into from
Oct 8, 2020
Merged

Unit test for CLI flag --name + doc update #1445

merged 7 commits into from
Oct 8, 2020

Conversation

kamolins
Copy link
Contributor

@kamolins kamolins commented Sep 22, 2020

Addresses: #1309

  • Added unit test to ensure that CLi flag --name supports regex
  • Updated docs

@davidjgoss
Copy link
Contributor

Thanks for doing this @kamolins, looks good.

Would you be able to add a test that fails without your change and passes with it?

@kamolins
Copy link
Contributor Author

@davidjgoss Done!

@kamolins kamolins changed the title CLI flag --name to support special characters CLI flag --name to match names with brackets Sep 24, 2020
@davidjgoss
Copy link
Contributor

davidjgoss commented Oct 5, 2020

@kamolins sorry, looking again at the documentation, this argument is listed as supporting regular expressions, so moving away from match will break that which I don't think we want to do (paging @charlierudolph in case I'm wrong).

You should be able to escape the parentheses and other characters that have regex meaning with backslashes, I think?

I do still think there is worthwhile change to make here if you wanted to pivot this PR a bit:

  • The doc briefly mention regex, but it might be worth showing an example with escaping
  • No tests broke when you changed from match to includes - we should add one that proves the regex support

@davidjgoss davidjgoss self-requested a review October 5, 2020 23:16
@kamolins
Copy link
Contributor Author

kamolins commented Oct 5, 2020

@davidjgoss You make a good point! includes won't match a regex, afaik. I should have read the docs more carefully. I will update the PR.

@kamolins kamolins changed the title CLI flag --name to match names with brackets Unit test for CLI flag --name + doc update Oct 6, 2020
@kamolins
Copy link
Contributor Author

kamolins commented Oct 6, 2020

@davidjgoss Updated the PR. Lmk if I should do anything else.

By the way, my previous push was marked as failed here on node v10 with an unrelated issue. Not really sure what went wrong there.

@davidjgoss davidjgoss merged commit be7e365 into cucumber:master Oct 8, 2020
@aslakhellesoy
Copy link
Contributor

Hi @kamolins,

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:

  • ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
  • 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
  • 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
  • ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
  • 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

Adam-ARK pushed a commit to Adam-ARK/cucumber-js that referenced this pull request Oct 21, 2020
* CLI flag --name to support special characters

* Added unit test for --name flag

* Removed semicolon

* Reverted changes. Added regex unit test. Updated docs.

* Rephrased sentence in docs

* use new bullet item for regexp example

Co-authored-by: David Goss <[email protected]>
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