-
-
Notifications
You must be signed in to change notification settings - Fork 592
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
Fix issues with allow_nil: true
#997
Conversation
@parndt I missed some necessary changes, this should resolve that. |
lib/friendly_id/finder_methods.rb
Outdated
raise_not_found_exception(id) unless allow_nil | ||
raise_not_found_exception(id) | ||
rescue => e | ||
allow_nil ? nil : raise(e) |
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 know we talked about exception control flow and I'm definitely not a fan, however I really am not aware of another strategy to use here. I'm open to suggestions though.
Dearest @parndt what is the likelihood of this getting merged and then a release? 🙏This PR satisfies the documentation. Let me know if you have feedback. Thanks! |
@joemsak I have been thinking about this change on and off all week! I've added a suggestion, let me know what you think |
I also spotted a duplicate |
Co-authored-by: Philip Arndt <git@p.arndt.io>
Thank you! |
Hey @parndt any chance on this making it to a release? Let me know your thoughts, thanks! |
@joemsak yes, definitely, I will get to it as soon as I can. 😄 |
Version 5.5.0 pushed @joemsak |
Turns out, the documentation I added in #995 wasn't 100% accurate. I had
to add tests for such examples and improve the internal code to support
the option. This should now provide the fully expected behavior.