ruby: introduce Gherkin::Query#parent_locations. #89
Conversation
|
So I've had another quick scan of this @botandrose - I think firstly this probably "feels" like a major change, because you're refactoring a huge chunk of how features e.t.c. are updated (I know they're private), but this entire area is a private DSL essentially. Looking at the code changes (Not spec), I actually think it all makes sense and is fine. Next step would be to review specs. |
|
@botandrose so I'm keeping you in the loop and working "semi" linearly. Are we saying this should / must go in first and be released before thinking about other fixes? |
ruby/lib/gherkin/query.rb
Outdated
| update_steps(background.steps) | ||
| parent_id = parent.respond_to?(:id) ? parent.id : parent.object_id # Cucumber::Messages::Feature doesn't have an id :( |
There was a problem hiding this comment.
Then I'm assuming this might need tweaking?
There was a problem hiding this comment.
IMO, the "correct" thing to do here would be to add a Cucumber::Messages::Feature#id method, to make it consistent with the rest of the message types, and then we could get rid of this workaround. It seems like an obvious hole to me. But it also seemed out of scope for this PR.
It seems to me that we have some options:
- I can push another commit adding
C::M::F#id - I can do a follow-up PR cleaning this up
I'm leaning towards 2 to avoid scope creep, but you're the boss! What do you think?
There was a problem hiding this comment.
@luke-hill Oh derp, 1 is impossible because its a different repo.
There was a problem hiding this comment.
@luke-hill I guess option 1 would be opening a PR on https://github.com/cucumber/messages , and waiting for that to be merged and released, and then we could merge and release this with a bumped requirement on that version, and then finally merge and release the cucumber-ruby-core PR with a bumped requirement on this.
There was a problem hiding this comment.
@luke-hill Just checked with crossed fingers, but C::M::Feature is still missing #id on current master : https://github.com/cucumber/messages/blob/main/ruby/lib/cucumber/messages.deserializers.rb#L244
https://github.com/cucumber/messages/blob/main/ruby/lib/cucumber/messages.dtos.rb#L471
There was a problem hiding this comment.
@luke-hill Ugh, and it looks like that code is all auto-generated. Adding an id to feature would change the data structure for ALL implementations of cucumber, not just the ruby one.
Maybe the simplest method would be to just use #object_id as the internal data structure key for both Feature and Rule. Or maybe just the object itself!
There was a problem hiding this comment.
@luke-hill Okay, I just pushed a commit that uses the object itself as the key instead, which ends up simplifying everything greatly, including removing the need for so many safe navigation operators. I'm happy with this if you are!
|
@luke-hill Yes, my cucumber-ruby-core PR is built on-top of this new API, so this has to land before that one can take advantage of it. And they are essentially a pair... without one, the other doesn't really make any sense. |
|
Approved CI to run. Think we're definitely 90% of the way there now. Sorry for the delay, I'm coming back after a long time off. |
|
@luke-hill all good! I really appreciate your time on this and my other contributions throughout the years. Cucumber is my most used testing framework, as well, so again, your time and effort are very much appreciated |
Cucumber::Core::Compiler will use this to tell Cucumber::Core::Test::Case the locations of its parent lines, such as `Feature:`, `Background:`, and `Rule:`, so that it will correctly match them during location filtering.
botandrose
force-pushed
the
parent_locations
branch
from
b2e3252 to
ae4a2bc
Compare
|
@luke-hill Okay, added a changelog entry, rebased on current main, squashed, force-pushed. I think this is ready for merge! |
|
Hi @botandrose, Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾 In return for this generous offer we hope you will:
On behalf of the Cucumber core team, |
|
This will go into the next major (v27), eta a week or so. |
|
@luke-hill @mattwynne thanks to both of you for your help on this! When v27 is out the door I'll rebase the other PR. |
|
Thank you @botandrose! |
🤔 What's changed?
Introduces a new Gherkin::Query#parent_locations method in the Ruby implementation.
Cucumber::Core::Compiler will use this method to tell Cucumber::Core::Test::Case the locations of its parent lines, such as
Feature:,Background:, andRule:, so that it will correctly match them during location filtering. This particular strategy was identified as potentially reasonable while pairing with @mattwynne.⚡️ What's your motivation?
This is one part of a two-repo PR to resolve the last remaining bits of the regression described in cucumber/cucumber-ruby#1469
🏷️ What kind of change is this?
📋 Checklist:
I'm not sure about these last two, since this could be considered as internals from a high-level cucumber point-of-view, but a public-interface addition from the gem point-of-view. Thoughts?