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

Fix/csharp cucumber expression support #65

Closed
wants to merge 4 commits into from
Closed

Fix/csharp cucumber expression support #65

wants to merge 4 commits into from

Conversation

Issafalcon
Copy link
Contributor

🤔 What's changed?

  • Updating c-sharp language support to be able to recognise cucumber expression syntax and non-verbatim string literal expressions

⚡️ What's your motivation?

🏷️ What kind of change is this?

  • ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

The ExpressionBuilder tests fail on the fact the string literal expressions are returned with double quotes instead of single quotes. I'm not sure how to get past this particular failure (or at least, where best to place the correction for it)

📋 Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
      - [ ] My change requires a change to the documentation.
      ~ - [ ] I have updated the documentation accordingly.~
  • [s] Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

{
}

[When(@"I have Regex parameters like (\d+) and (\d+)")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this or make it consistent with step definitions for the other languages. Ref point 4 in CONTRIBUTING.md.

There is a test that expects exact expressions:

assert.deepStrictEqual(expressions, ['a {uuid}', 'a {date}', /^a regexp$/])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I'll update this. What are your thoughts on this [When(@"I have Regex parameters like (\d+) and (\d+)")] expression out of interest? It is a common pattern in the Specflow generated step definitions (using the Visual Studio Specflow extension), so I'm wondering if it's OK to not have it included here as part of the testing.

Copy link
Contributor

@aslakhellesoy aslakhellesoy left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this! I've left a comment about what needs to be fixed.

@aslakhellesoy
Copy link
Contributor

aslakhellesoy commented Jun 3, 2022

Also, please add c_sharp to

const parameterTypeSupport: Set<LanguageName> = new Set(['typescript', 'java', 'ruby'])

- Still seems to fail on single vs double quoting the expected outputs
@Issafalcon Issafalcon requested a review from aslakhellesoy June 7, 2022 07:42
@Issafalcon
Copy link
Contributor Author

Thanks for the review @aslakhellesoy ! The tests still fail on the expectations of a double quoted string as opposed to single quotes. I'm not sure what the best way to resolve this is.

@aslakhellesoy
Copy link
Contributor

Superseded by #68

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants