-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Restore support for matching a scenario by multiline step argument line numbers #239
Conversation
@luke-hill @mattwynne I have some free time coming up if this needs any work. WDYT? |
I have probably done about 2-3 hours on cucumber over the last month, owing to real life stuff. I can maybe get around to contributing here or offering some insight later in the month. If you're ok to continue solo I can check it in a while. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Some minor suggestions for tweaks but this looks great, thanks for figuring it out!
lib/cucumber/core/test/step.rb
Outdated
@@ -51,6 +51,14 @@ def action_location | |||
@action.location | |||
end | |||
|
|||
def matching_locations | |||
if multiline_arg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the multiline_arg
prop defaults to an Test::EmptyMultilineArgument
(a null object) so I don't think you need this conditional.
lib/cucumber/core/test/location.rb
Outdated
@@ -79,6 +79,13 @@ def to_str | |||
to_s | |||
end | |||
|
|||
def select_down(lines_count) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this!
What about just calling it select
and taking the thing that has a lines_count
. Might read a bit nicer on the calling code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or merge
?
[location.merge(multiline_arg)]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah actually select
has other meanings in Ruby
…ltiline_arg is always truthy.
@mattwynne Okay, just pushed a commit incorporating your suggestions. Ready to merge? Then I can move onto a new PR plugging the last few holes in the larger scenario-matching regression. |
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, |
Superb, thanks @botandrose! |
You can use branches in this repo now for PRs, if you prefer. |
Description
The Cucumber 4.0 release introduced some regressions in matching scenarios by line numbers other than the exact scenario line. The feature line, tag line, background lines, and step lines all stopped matching their related scenario(s). More context at cucumber/cucumber-ruby#1469
This PR builds on top of #238 to also match multiline arguments like tables and docstrings. Note that I didn't update the CHANGELOG because I think this falls within the changelog line added in the previous PR.
Type of change
Checklist:
bundle exec rubocop
reports no offenses