Skip to content
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

fixed #notifiable_path #45

Closed
wants to merge 1 commit into from
Closed

Conversation

upstill
Copy link
Contributor

@upstill upstill commented Nov 19, 2017

#notifiable_path would work if the notifiable class actually defined it. However, if it was defined in a superclass, the Notifiable module's override of the method would ignore it and throw an error. Now a superclass can define #notifiable_path and it will get called.

#notifiable_path now calls super#notifiable_path if it exists
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 99.939% when pulling 4a109d6 on upstill:development into bf6dad4 on simukappu:development.

1 similar comment
@coveralls
Copy link

coveralls commented Nov 19, 2017

Coverage Status

Coverage decreased (-0.06%) to 99.939% when pulling 4a109d6 on upstill:development into bf6dad4 on simukappu:development.

@simukappu
Copy link
Owner

Hi @upstill

This bug fix is good, but I have a few comments for your pull request. Could you update this?

  • if defined?(super) seems to be duplicated with defined?(super) ? super : . Simplify your code.
  • Write a test. Test coverage decreased.

Thanks!

@simukappu
Copy link
Owner

I have added your request in adobe referenced commit and release them as v1.5.1. I will close this pull request.
Thank you for your contribution!

@simukappu simukappu closed this Aug 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants