search

cucumber / cucumber-rails

Rails Generators for Cucumber with special support for Capybara and DatabaseCleaner
https://github.com/cucumber/cucumber-rails
MIT License
1.02k stars 327 forks source link

Use `prepend` instead of alias method chaining #454

Closed wagenet closed 4 years ago

wagenet commented 4 years ago

In multiple places alias method chaining is used instead of prepend. Since cucumber-rails doesn't work with version of Ruby prior to 2.0, these should be updated to the more semantically correct prepend.

luke-hill commented 4 years ago

I have a quick precursory glance for alias_method_chain and couldn't find any. There are 4 calls to alias_method which are used to "patch" up click to use / not use js. Are those what you mean?

wagenet commented 4 years ago

Sorry, I should have been more specific. I was referring to: https://github.com/cucumber/cucumber-rails/blob/890dae75415d429f22168c4bf022e6ac7cc6822e/lib/cucumber/rails/action_controller.rb#L9 https://github.com/cucumber/cucumber-rails/blob/8b162dcd6d27c59cf438d580384acac3f1f183d5/lib/cucumber/rails/application.rb#L12

There might be a better way to handle it in the JS stuff, but given that you're doing execute-time changes, it might not be possible to do it with prepend.

luke-hill commented 4 years ago

ah right, I see there, because we alias it but then call it. That makes sense yes. Those 2 look like they could be tweaked. But I've not dived in enough (But I have a half-decent knowledge of prepend).

The two JS ones I don't think you can reliably alter, what they do isn't pretty. But it means the front-end user has to literally do a 1line tag to make the behaviour very different, which is highly desirable.

wagenet commented 4 years ago

@luke-hill if this sounds ok to you, I can do a PR for the first non-JS ones.

luke-hill commented 4 years ago

Sure thing. Thanks for that :+1: