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

Handle case when nil is explicitly passed to fetch #25

Merged
merged 1 commit into from
Oct 17, 2015

Conversation

dasnixon
Copy link
Contributor

I ran in to this issue while using the active model serializers gem
version v0.10.0.rc3.

They default their options that get passed to the cache as nil if
none are set. Using an options hash doesn't guard against explicitly
passed parameters, in this case nil. That caused an error, shown in
the screenshot below.

I hope this helps :)

screen shot 2015-10-15 at 11 56 14 pm

@@ -156,6 +156,13 @@ def self.load(value)

expect(cache.read('short-key')).to eq('other stuff')
end

context 'handling passing nil to options' do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spec looks right on. Can you drop the context and put more detail into the spec description instead?

@sorentwo
Copy link
Owner

Thanks for the fix. That pesky nil messing everything up again!

I left a couple of minor notes, but can you also add a changelog entry?

I ran in to this issue while using the active model serializers gem
version v0.10.0.rc3.

They default their options that get passed to the cache as nil if
none are set. Using an options hash doesn't guard against explicitly
passed parameters, in this case nil. That caused an error, shown in
the screenshot below.

I hope this helps :)
@dasnixon
Copy link
Contributor Author

@sorentwo updated, let me know if there is anything else. Appreciate the fast response and the awesome gem! 👍

sorentwo added a commit that referenced this pull request Oct 17, 2015
Handle case when nil is explicitly passed to fetch
@sorentwo sorentwo merged commit 08f7909 into sorentwo:master Oct 17, 2015
@sorentwo
Copy link
Owner

Thanks for the patch @dasnixon! Glad you're getting use out of the gem =)

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

2 participants