search

erikedin / Behavior.jl

Tool for Behavior Driven Development in Julia
Other
25 stars 3 forks source link

Lenient parser: Optionally allow mid-line comments #59

Open erikedin opened 3 years ago

erikedin commented 3 years ago

The Gherkin reference says this:

Comments are only permitted at the start of a new line, anywhere in the feature file. They begin with zero or more spaces, followed by a hash sign (#) and some text.

A scenario is therefore not allowed to have mid-line comments, like

@tag # Some comment
Scenario: Some description

We could add a parse option "Allow mid-line comments". We should make a note that this option ought to be a last resort, since that will break properly written scenarios that use a # anywhere, like

Scenario: Some description
    Given that issue #58 is fixed

as this step would then only match "Given that issue", without the rest of the line. Also, we should probably not treat # in descriptions as comments.

Alternative: Make much more granular options, for specific parts, like "Allow mid-line comments for tags", "... for scenario steps", "... for examples".

Cucumber appears to successfully parse the feature file in this scenario, but does not give the scenario the intended tag.

erikedin commented 3 years ago

I had a brief look at what Cucumber does in this scenario, and it looks a bit undefined, which is fine. It doesn't properly parse the tag, but it also doesn't give an error.

tk3369 commented 3 years ago

I can't seem to find any mid-line comments with Grakn's features... Maybe this is not needed? Let me know otherwise.

erikedin commented 3 years ago

I have a pull request in the behaviour repo to fix the ones I found (https://github.com/graknlabs/behaviour/pull/177), but for instance this one one line 1137 in graql/language/delete.feature.

  @ignore # TODO: enable once query planning has been optimised

I'm not sure how to handle those, though. I did some experiments, and Cucumber doesn't throw an error on lines like these, but it doesn't seem to correctly parse the tag either. That's why I made a PR, since the @ignore tag doesn't work as expected. So I can see a few options:

  1. We allow it, and ensure that tags are properly parsed
  2. We allow it, but leave the results undefined (as Cucumber does)
  3. We continue to throw an error (this would follow the reference documentation the closest I guess)

Option 1 isn't that hard, so it's maybe the best option. Since Cucumber doesn't fail on lines like this, I'm not sure I want to fail. But I definitely don't want undefined behavior (option 2), since that will lead to scenarios running or not running unexpectedly.

As long as my PR is accepted, it probably won't be a major issue, so I think we have time to decide.

mkschulze commented 3 years ago

As long as my PR is accepted, it probably won't be a major issue, so I think we have time to decide.

It has been approved by two reviewers already, so I guess it won't take them too long until it get's merged.

tk3369 commented 3 years ago

I agree. Grakn should be motivated to keep their code portable across various BDD systems.

mkschulze commented 3 years ago

Hey @erikedin , the last reviewer James has approved the PR, he just asks to resolve the remaining conflicts. Then they will merge it. Thx!

mkschulze commented 3 years ago

thank you @erikedin