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

Gherkin: Remove Background from Rule in grammar #590

Closed
ciaranmcnulty opened this issue Apr 3, 2019 · 9 comments
Closed

Gherkin: Remove Background from Rule in grammar #590

ciaranmcnulty opened this issue Apr 3, 2019 · 9 comments

Comments

@ciaranmcnulty
Copy link
Contributor

@ciaranmcnulty ciaranmcnulty commented Apr 3, 2019

Summary

Based on conversation at Cukenfest, we will take out Background from Rules and see if valid use cases emerge

Expected Behavior

This will become a parse error:

Feature: My Feature
    Rule: My Rule
        Background: My background

Current Behavior

Currently backgrounds are inlined into the pickles created from the Scenarios

@aslakhellesoy

This comment has been minimized.

Copy link
Contributor

@aslakhellesoy aslakhellesoy commented Apr 18, 2019

I had a crack at this (removed Background? from the Rule grammar rule).

Because of the way Berp works, a rule with a background does not cause a parser error. It just generates a Rule AST node with description Background:\n Given rb.

The Berp parser will only attempt to match lines it expects. Everything else becomes a description. I think parser is unable to detect quite a lot of parse errors. /cc @gasparnagy

@ttutisani

This comment has been minimized.

Copy link

@ttutisani ttutisani commented Apr 25, 2019

Hi. I came here as I was trying to understand whether Gherkin AST produces Rule objects or not. I know that it was introduced as of Gherkin 6, but the AST picture on the start page of this GitHub project does not yet mention Rule in it. So, it is supported yet?

@gasparnagy

This comment has been minimized.

Copy link
Member

@gasparnagy gasparnagy commented Apr 29, 2019

@ttutisani It is supported, but the docs have not been updated yet, I guess, but please create a separate issue for this.

@gasparnagy

This comment has been minimized.

Copy link
Member

@gasparnagy gasparnagy commented Apr 29, 2019

@aslakhellesoy I see. It was definitely by design like that, but we can question whether this was really a good choice (I am not sure anymore.) I think besides "Background" and maybe "Feature" all the other keywords can be used pretty much everywhere, so the impact of this behavior is not that high.

If you write "Background" after a "Rule" and start with the first "Given" step, it will anyway cause a synxtax error (if we don't allow backgrounds for rules), because at that point only "Scenario" or another "Rule" can come, so there will be no hidden error caused by that.

I would go ahead removing the Background for the Rule and maybe have another separate discussion about how the parser should treat the "other" token. We can easily change BERP (if we want) to check every other token and only accept "Other" if no other token is matching. This will not require any change in the language implementations.

@aslakhellesoy

This comment has been minimized.

Copy link
Contributor

@aslakhellesoy aslakhellesoy commented Apr 29, 2019

That sounds like a good plan @gasparnagy - good to hear berp can be easily changed.

@stale

This comment has been minimized.

Copy link

@stale stale bot commented Aug 26, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

@stale stale bot added the Stale label Aug 26, 2019
@stale

This comment has been minimized.

Copy link

@stale stale bot commented Sep 2, 2019

This issue has been automatically closed because of inactivity. You can support the Cucumber core team on opencollective.

@stale stale bot closed this Sep 2, 2019
@aslakhellesoy

This comment has been minimized.

Copy link
Contributor

@aslakhellesoy aslakhellesoy commented Jan 8, 2020

IIRC the rationale for not allowing Background in Rule was that it would provide too much rope for people to hang themselves with. It would be easy to create a mess that's hard to reason about, and we want to avoid that.

@sealr00t

This comment has been minimized.

Copy link

@sealr00t sealr00t commented Mar 23, 2020

I would suggest to keep Background inside Rule's as it provides a very good solution for having separate/multiple Background keywords inside one feature file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.