search

piotrmurach / loaf

Manages and displays breadcrumb trails in Rails app - lean & mean.
MIT License
407 stars 24 forks source link

Remove the need to supply the controller to the Proc #22

Closed brendon closed 6 years ago

brendon commented 6 years ago

When doing dynamic breadcrumb names and urls we can execute the Proc in the context of the controller so that it doesn't need to be explicity supplied.

Backward compatibility is maintained by checking for the block's arity and sending the controller if necessary.

Closes #21 and #20

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.4%) to 96.667% when pulling 5072707c99fc439e3e581abb136c696b37df416f on brendon:instance_exec into ef94f7e45e44cdbf60eff46df015c38e1fed0ff1 on piotrmurach:master.

brendon commented 6 years ago

Hound is such a drag! ;)

piotrmurach commented 6 years ago

Hi Brendon,

This is a great idea! I would skip fixing hound issues as they are more suggestions than things to abide by. Especially, I wouldn't apply any suggestions that prevent code from working on old Rubies. Once Travis CI is back in green I will merge!

brendon commented 6 years ago

Thanks @piotrmurach :) I think the travis test suite is brittle. The failures are due to rubies not installing etc... as far as I can see. I'll look into how we can get it more reliable. I'm a maintainer for acts_as_list and we use travis reliably for that so hopefully it's something simple.

brendon commented 6 years ago

What a nightmare! However I think I've tidied things up and you should get a green suite provided we don't get any timeout issues which I noticed have happened a couple of times, but I think this is a problem on the travis end.

You'll see the travis.yml file has been overhauled. The exclusions weren't actually working, and the inclusions only tested those ruby versions against rails 3.2 so I've removed testing against head, and added jruby as a first-class citizen though it can only be tested on Rails 3.2 - 5.1.

I hope it's green, if you get a fail due to a timeout, can you re-run the suite for me?

piotrmurach commented 6 years ago

Thanks, I do appreciate how frustrating the Travis testing setup is with all the Rails as I had quite a few issues setting it up in the first place. The latest build passes and I had a quick review of your changes and I think they are good to merge! I should have some time tomorrow to work on this, please hold tight!

brendon commented 6 years ago

Thanks @piotrmurach :) It was a fun change to make. Learned some new things such as instance_exec! :) Thanks for looking to merge this. Would you be able to do a gem release shortly after? Then I can get on with integrating loaf into my application.

TheNeikos commented 6 years ago

Great that this got implemented! Thanks :+1:

piotrmurach commented 6 years ago

Thanks for ideas and work on this @brendon & @TheNeikos

Released v0.7.0 🍾 Enjoy!

brendon commented 6 years ago

Thanks @piotrmurach! :) Great to be able to add something helpful :D