Changes to parser to pass all fixture/good tests

[mladedav]

There are a lot of changes to the parsing and I admit some of them are not ideal, but they have precedent in the data. I am kind of stumped that Gherkin does not have some kind of official regex and even the specification itself does not clearly state all things that should be parsed but I know I'm barking at the wrong tree here.

This includes changes in #31.

To pick a few of the worst things, as per gherkin documentation about Description: You can write anything you like, as long as no line starts with a keyword. which however breaks current smoke2 test which starts feature description lines with * and but. A case could be made that each description (and there can be descriptions for Feature, Example/Scenario, Background, Scenario Outline and Rule) could be made context-aware in the sense that only the subset of keywords that may follow the given described keyword would be considered as ending the description.

Another sub-par thing is that an empty file (or a file with just comments) is considered valid feature file, for which I had to return default feature, i.e. no rules, no steps, empty name. Returning Option<Feature> would be probably better, but it would require non-trivial changes as well as changes to downstream services (i.e. cucumber).

Also steps may now start with a keyword without a space, e.g. Butterscotch would now be a valid step parsed as But: terscotch. This is because of a fixture with the emoji language. It turns out people using emojis seem to not use spaces and I didn't feel like introducing logic to require spaces only for this language.

I kind of like the experience with peg I'm getting so I would love to help some more (maybe getting rust cucumber to be semi-official implementation) so if there's anything I would love to know.

[ilslv]

@mladedav

I am kind of stumped that Gherkin does not have some kind of official regex and even the specification itself does not clearly state all things that should be parsed but I know I'm barking at the wrong tree here.

Yeah, thats really unfortunate, we suffer from this all the time 😔

as per gherkin documentation about Description: You can write anything you like, as long as no line starts with a keyword.

To be fair I don't really trust Gherkin reference on cucumber.io as it's pretty outdated and skips some caveats for simplicity. Thats ok for general user, but doesn't work for implementors.

A case could be made that each description (and there can be descriptions for Feature, Example/Scenario, Background, Scenario Outline and Rule) could be made context-aware in the sense that only the subset of keywords that may follow the given described keyword would be considered as ending the description.

I like that way more. So instead of using general purpose description() parsing rule for all descriptions, we should make special parsing rule for each one.

Another sub-par thing is that an empty file (or a file with just comments) is considered valid feature file, for which I had to return default feature, i.e. no rules, no steps, empty name. Returning Option would be probably better, but it would require non-trivial changes as well as changes to downstream services (i.e. cucumber).

👍

Also steps may now start with a keyword without a space, e.g. Butterscotch would now be a valid step parsed as But: terscotch. This is because of a fixture with the emoji language. It turns out people using emojis seem to not use spaces and I didn't feel like introducing logic to require spaces only for this language.

Actually, I don't think you are right. The way this is handled is that space is a part of keyword. Emoji language definition clearly shows that * has a trailing space and 🙏 doesn't

[mladedav]

Actually, I don't think you are right. The way this is handled is that space is a part of keyword. Emoji language definition clearly shows that * has a trailing space and 🙏 doesn't

You're right, the issue why this did not work is that this repo has possibly outdated languages.json without the spaces. With them this works properly.

[mladedav]

I added the description excludes per keyword. Maybe some could be contracted (e.g. scenario and scenario outline) into a single function, but I decided to play it safely. I also added just the ones that seemed logical (e.g. scenario has to have steps---the data seem like that's true but I would not be surprised if it wasn't).

I had some troubles with the borrow checker (as I do in virtually every pull request I open) and I ended up cloning the keywords iterable in the keyword rule because of lifetimes. If there is another way to do this that would be great. The only thing that came to mind was creating arrays of keywords to be skipped through build.rs same as the Keyword structures are made, but that felt too cumbersome.

I also noticed that this should fix #10.

I will hopefully look into validating the parsing results from the fixtures because as it is now it could theoretically just parse the top level Feature keyword and treat everything else as description and we would never know (except for the parts that are tested manually), so until that is done some of these changes could be considered a bit dangerous.

[tyranron]

ping @mladedav

Are you going to continue work on this, or we should pick it up?

[mladedav]

Sorry I didn't get back to you on this sooner.

I plan to finish this, I hope to get some work here done during the week.

[mladedav]

Sorry it took longer than I had hoped, but I am fairly happy with the extent that the AST is checked against the fixtures. It uncovered a few bugs concerning which keywords can be used in a description so I guess it was worthwhile.

The AST parsing checks just backgrounds, scenarios, and rules. There are no checks for stuff like tags.

[ilslv]

@mladedav so this PR is ready for review?

[mladedav]

Yes, it is ready.

[tyranron]

@mladedav thanks 🍻

[tyranron]

@mladedav released in 0.12.0.

[tyranron]

@mladedav released in cucumber 0.13.0.