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.

Sign up for GitHub

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

Jump to bottom

Add gherkin query in Ruby #845

Merged
merged 9 commits into from
Jan 9, 2020
Merged

Add gherkin query in Ruby #845

merged 9 commits into from
Jan 9, 2020

Conversation

vincent-psarga
Copy link
Contributor

Summary

Like this was done in Gherkin-JS, this PR adds a simple GherkinQuery object that provides the Location objects of various AST nodes found in GherkinDocument message after parsing.

Also, it raises an exception when we iterate more than once on the messages. If we don't do so, we may end up with multiple AST node objects representing the same part of the Gherkin feature but with different IDs.

This object will likely grow in the future (for example for finding the original PickleStep location), but this should already be enough to be used in cucumber-ruby(-core).
Maybe more will be added to this PR or in other ones when needed.

How Has This Been Tested?

With Unit tests

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

  • The change has been ported to Java.
  • The change has been ported to Ruby.
  • The change has been ported to JavaScript.
  • The change has been ported to Go.
  • The change has been ported to .NET.
  • I've added tests for my code.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

luke-hill
luke-hill reviewed Jan 8, 2020
View reviewed changes
Copy link
Contributor

@luke-hill luke-hill left a comment

Choose a reason for hiding this comment

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

Looks cool. I can see how this would start giving us more powerful capabilities when it comes to the custom parts we can add post gherkin-9

gherkin/ruby/lib/gherkin/gherkin_query.rb Outdated
update_feature(message.gherkin_document.feature) if message.gherkin_document
end

def get_location(ast_node_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

are these methods "synched up" across all implementations. If they are that's cool. If not traditionally we would drop get_ and set_ prefixes in methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

We aim to have a consistent API between implementations, but I still think we should be idiomatic. Please change to location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

gherkin/ruby/lib/gherkin/errors.rb Outdated
@@ -1,5 +1,7 @@
module Gherkin
class ParserError < StandardError; end
class AstNodeNotLocatedException < Exception; end
Copy link
Contributor

Choose a reason for hiding this comment

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

traditionally errors in ruby all end in error (I'm aware that there are existing ones here that don't), so it's not a biggie if you don't fix

Copy link
Contributor

Choose a reason for hiding this comment

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

Custom exceptions should extend from StandardError. Never extend Exception.

Copy link
Contributor Author

@vincent-psarga vincent-psarga Jan 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

inheritance is done.
For the Error vs Exception, all (except ParserError) are called XXXException, so I went with the rest

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense for now yeh, top work though, this stuff should get gherkin 9 ready soon :)

aslakhellesoy
aslakhellesoy suggested changes Jan 8, 2020
View reviewed changes
gherkin/ruby/lib/gherkin/errors.rb Outdated
@@ -1,5 +1,7 @@
module Gherkin
class ParserError < StandardError; end
class AstNodeNotLocatedException < Exception; end
Copy link
Contributor

Choose a reason for hiding this comment

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

Custom exceptions should extend from StandardError. Never extend Exception.

gherkin/ruby/lib/gherkin/gherkin_query.rb Outdated
update_feature(message.gherkin_document.feature) if message.gherkin_document
end

def get_location(ast_node_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

We aim to have a consistent API between implementations, but I still think we should be idiomatic. Please change to location.

aslakhellesoy
aslakhellesoy suggested changes Jan 8, 2020
View reviewed changes
gherkin/ruby/lib/gherkin/gherkin_query.rb Outdated
@@ -0,0 +1,54 @@
module Gherkin
class Query
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename the file to query.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@vincent-psarga
Copy link
Contributor Author

Note: I'm giving a bit more thoughts on #argument_location and it may not be the way to go (it's more a hack that anything else).

I think PickleDocString and PickleTable messages should both have an ast_node_id field, referencing the DocString and DataTable messages (which should also have their own ids, obviously).
@aslakhellesoy that sound good for you ? I can make a second PR for that.

@vincent-psarga vincent-psarga mentioned this pull request Jan 8, 2020
Merged
6 tasks
@vincent-psarga
Copy link
Contributor Author

vincent-psarga commented Jan 8, 2020 •
edited
Loading

I can make a second PR for that.

which is there (#847) and in progress

@aslakhellesoy aslakhellesoy merged commit b8c9b57 into master Jan 9, 2020
@aslakhellesoy aslakhellesoy deleted the add-ruby-gherkin-query branch January 9, 2020 10:25
@vincent-psarga vincent-psarga mentioned this pull request Jan 9, 2020
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luke-hill luke-hill luke-hill left review comments

@aslakhellesoy aslakhellesoy aslakhellesoy approved these changes

Assignees
No one assigned
Labels
language: ruby library: gherkin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vincent-psarga @aslakhellesoy @luke-hill