Makes it possible to add extra spaces between the Action and the text…

[gerben86]

…. This can make the feature files more readable. #1732

Description

Thanks for contributing this Pull Request. Make sure that you submit this Pull Request against the develop branch of this repository, add a brief description, and tag the relevant issue(s) and PR(s) below.

• Relevant Issues : It's not possible to put extra spaces between the variable and the assignment #1732
• Relevant PRs : (optional)
• Type of change :
  • New feature
  • Bug fix for existing feature
  • Code quality improvement
  • Addition or Improvement of tests
  • Addition or Improvement of documentation

Modified the regex for the ScenarioActions so that each Action can be executed with spaces around it. But there should be at least 1 whitespace.
Also added a test to prove that it works. Test can be removed of incorperated in the existing testcases ofcourse.
This can improve the readability of the feature files.

And I thought earlier about raising this as an improvement, but now it was the right time for me :)

[ptrthomas]

@gerben86 there are build failures

[gerben86]

I fixed one.
The other failure I don't understand. Could you give me a headsup on 'testcontinueOnStepFailure3'?

[ptrthomas]

@gerben86 sorry I don't have time right now - cc @joelpramos since he contributed that test

[gerben86]

Ok, got it. I don't know why it only failed in this scenario...

[ptrthomas]

I have a few questions and I'll ask others for opinion as well before merging this cc @peterquiel

• does this affect the IDE auto-complete user-experience for Gherkin in Eclipse and IntelliJ
• does this regex negatively affect performance of the step-matcher at run-time
• until now we seemed to be fine, and in my personal opinion enforced a standard across all teams.
• I am assuming that we still enforce at least one space to surround a = or == or contains etc.

[gerben86]

1: Can someone else check it? I don't use the auto-complete for Gherking in Eclipse/Intellij
2: I can't imagine that it has a noticable performance impact, because still the regex-matcher is used, although the regex may be a little bigger. I did a check locally (Windows, 3 year old machine), and on 2000 calls for the new implementation of 'def' the total run took +/- 50ms longer then with the old implementation.
3: I don't see the benefit of this standard, I think scripts can be easier readable if there are more whitespaces, especially if your using multiple def/match statements. But that's up to the user to make use of it or not.
4: Correct, still at least one space is enforced

[edwardsph]

Is there any reason for def and status to be different to the others:
e.g. def \\h* vs def\\h+
I think it would be good to be consistent.

[gerben86]

Is there any reason for def and status to be different to the others:
e.g. def \\h* vs def\\h+
I think it would be good to be consistent.

Good catch, the origin of this lies in the test "testcontinueOnStepFailure3".
That fails if it's consistent with the others. The strange thing is that ONLY this test is failing, all others are passing...

[joelpramos]

Not sure when I'll have time to pull your branch but looked at the error log and through the code and I suspect it has to something to do with configuring specific keywords that are to be ignored when there are failures. On the Config class there should be a section configuring the continueOnStepFailure, can you maybe check there if anything weird pops up?

[joelpramos]

@gerben86 change StepRuntime.java:75 to regex.substring(1).split(" |\\\\h|\\\\s")[0]; and feel free to remove the spaces on the step definitions you added to overcome the issue (I think in the status and def keywords).

[gerben86]

@gerben86 change StepRuntime.java:75 to regex.substring(1).split(" |\\\\h|\\\\s")[0]; and feel free to remove the spaces on the step definitions you added to overcome the issue (I think in the status and def keywords).

That was indeed the correct fix! Just updated the code, thanks!

[ptrthomas]

@gerben86 I think I merged too soon, can you submit another PR with the extra changes

[gerben86]

Yes, new one created.