cucumber/cucumber-ruby-core

After hooks should be run in reverse order

Closed this issue · 6 comments

erran-r7 commented

@mattwynne I can think of a couple of ways to change this behavior after looking at the code for a few minutes:

  • Use Array#unshift instead of Array#<< when adding new after hooks.
    • This has the drawback of requiring that @after_hooks is always appended to using #unshift vs. the more idiomatic #<<.
  • It looks like updating the CaseMapper#after_hooks getter method could be a dirtier way to fix this.
    • Use Array#reverse in CaseMapper#after_hooks to return the AfterHook array in reverse order.
    • This has the benefit of being able to use the more idiomatic #<< and would possibly prevent confusion when adding new elements to the array.

What's your opinion? I'd be willing to write a PR for either.

brasmusson commented

I can't see that changing CaseMapper#after_hooks could work, because that method is used when adding after hooks in DSL#after (making it adding to a reverse copy of @after_hooks).

That leaves using #unshift, it is less idiomatic than #<<, but I do not find any other place that hooks are added than in DSL#after.

erran commented

@brasmusson I'll test out using #unshift later today then. I didn't realize that DSL#after also used #after_hooks (I initially noticed its usage in Mapper#testcase).

mattwynne commented

Yeah this code is maybe a bit unintuitive. It would be nicer to separate the interface used by DSL for capturing hooks from the one used for pulling them out in the Mapper. Having an explicit reverse would definitely be a sweeter solution.

Still, I think we have bigger fish to fry. Thanks for your contribution @erran!

erran-r7 commented

@mattwynne Happy to help! 👍

lock commented

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.