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

Bootstrap AST and parser #1

Merged
merged 4 commits into from
Nov 18, 2021
Merged

Bootstrap AST and parser #1

merged 4 commits into from
Nov 18, 2021

Conversation

tyranron
Copy link
Member

@tyranron tyranron commented Nov 6, 2021

Moved from cucumber-rs/cucumber#153
Part of cucumber-rs/cucumber#124

Synopsis

This is the first PR in the story of Cucumber expressions support.

Solution

This PR implements Cucumber expressions AST and a parser for it. Next PRs will be adding AST into regex transformation and support inside proc-macros.

Checklist

  • Created PR:
    • In draft mode
    • Name contains Draft: prefix
    • Name contains issue reference
    • Has assignee
  • Documentation is updated (if required)
  • Tests are updated (if required)
  • Changes conform code style
  • CHANGELOG entry is added (if required)
  • FCM (final commit message) is posted
    • and approved
  • Review is completed and changes are approved
  • Before merge:
    • Milestone is set
    • PR's name and description are correct and up-to-date
    • Draft: prefix is removed
    • All temporary labels are removed

@tyranron tyranron added the enhancement New feature or request label Nov 6, 2021
Optional(Optional<Input>),

/// Text.
Text(Input),
Copy link
Member Author

Choose a reason for hiding this comment

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

This deviates from the original grammar by missing the parameter variant.

Need to discuss this on voice.

Copy link
Member

Choose a reason for hiding this comment

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

@tyranron there is a Note: section which says

While parameter is allowed to appear as part of alternative and option in the AST, such an AST is not a valid a Cucumber Expression.

Basically ARCHITECTURE.md describes AST that tries to be wider than Cucumber Expression for some reason (the only I can think of is to make parser implementation simpler and then error on conversion to real Cucumber Expression).
But in reality it doesn't make much sense to me. Especially alternative definition

alternative         = optional | parameter | text
text                = whitespace | ")" | "}" | .

This grammar suggests that alternative may have unescaped whitespaces, which is not true: example. They have to be escaped at least to avoid ambiguity.

Copy link
Member

@ilslv ilslv Nov 8, 2021

Choose a reason for hiding this comment

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

It's not even EBNF, as I understand, as wikipedia says that repetition is described with {...} and not with (...)*. It looks more like regex, but still has , for concatenation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ilslv would you be so kind to make a PR to upstream that adjusts the described grammar to be accurate and precise enough. Because it really bothers: having spec which doesn't reflect reality, while implementations don't follow the spec 😕

Copy link
Member Author

@tyranron tyranron Nov 8, 2021

Choose a reason for hiding this comment

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

@ilslv

It's not even EBNF, as I understand, as wikipedia says that repetition is described with {...} and not with (...)*. It looks more like regex, but still has , for concatenation.

From your link: * is a repetition, and ( ... ) is grouping. So we have a group repetion here. I don't see any mistakes in that. And in Markdown it has ```ebnf notation 🤷‍♂️

src/parse.rs Outdated
/// ```text
/// parameter := '{' (name | '\' name_to_escape)* '}'
/// name := ^name_to_escape
/// name_to_escape := '{' | '}' | '(' | '/' | '\'
Copy link
Member Author

Choose a reason for hiding this comment

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

It's better to preserve here the original EBNF grammar syntax, so it will match the architecture document, and preserve original naming asap.

Markdown can do EBNF pretty well, though:

parameter = "{" (name | "\" name_to_escape)* "}"

Copy link
Member

Choose a reason for hiding this comment

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

As described above, provided EBNF grammar tries to describe language that is wider than Cucumber Expression, but has errors. Grammar in my PR describes exactly Cucumber Expression.

@tyranron
Copy link
Member Author

tyranron commented Nov 6, 2021

ack @ilslv

As I've re-created this PR and represent its author, GitHub refuses to let me review it. So, please, just ping me for the next review.

@ilslv
Copy link
Member

ilslv commented Nov 9, 2021

@tyranron can you add me as collaborator in this repo, so I can push AST into Regex transformation branch?

@tyranron
Copy link
Member Author

tyranron commented Nov 9, 2021

@ilslv here you go

@ilslv
Copy link
Member

ilslv commented Nov 9, 2021

@tyranron for some reason i still can't push a new branch

16:18:17.613: [cucumber-expr] git -c credential.helper= -c core.quotepath=false -c log.showSignature=false push --progress --porcelain origin refs/heads/ast-to-regex-transformation:ast-to-regex-transformation --set-upstream
remote: Permission to cucumber-rs/cucumber-expressions.git denied to ilslv.
fatal: unable to access 'https://github.com/cucumber-rs/cucumber-expressions.git/': The requested URL returned error: 403

@ilslv
Copy link
Member

ilslv commented Nov 17, 2021

@tyranron should I prepare this PR for review or send all at once in #2?

@tyranron
Copy link
Member Author

@ilslv let's merge them separately.

@ilslv
Copy link
Member

ilslv commented Nov 18, 2021

FCM

Cucumber expression AST and parser (#1)

@ilslv
Copy link
Member

ilslv commented Nov 18, 2021

@tyranron ready for review

@tyranron tyranron added this to the 0.1 milestone Nov 18, 2021
Copy link
Member Author

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@ilslv thanks! Quite a brilliant code! 💪

@tyranron tyranron merged commit b71500c into main Nov 18, 2021
@tyranron tyranron deleted the ast-and-parser branch November 18, 2021 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants