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

Add fetch default value for options hash #1121

Merged
merged 1 commit into from
Apr 20, 2018

Conversation

amrani
Copy link
Contributor

@amrani amrani commented Mar 18, 2018

As stated in the documentation, a Field::Polymorphic should default its classes to an empty array.

  • Default classes for a Field::Polymorphic
  • Add spec coverage

@@ -30,7 +30,7 @@ def associated_dashboard(klass = data.class)
end

def classes
options.fetch(:classes) || []
options.fetch(:classes, [])
Copy link
Contributor

Choose a reason for hiding this comment

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

.fetch raises a KeyError when the key is missing, it doesn't return false, so the second part of the conditional would have never ran in the previous code:

2.4.0 :001 > hash = {}
 => {} 
2.4.0 :002 > res = hash.fetch(:key1) || 'Something'
KeyError: key not found: :key1
	from (irb):2:in `fetch'
	from (irb):2
	from /Users/fana/.rvm/rubies/ruby-2.4.0/bin/irb:11:in `<main>'
2.4.0 :003 > res
 => nil 

I'm wondering if users have been rescuing KeyError as a result and this will be a breaking change for them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think if someone were rescuing then it would be to return a default. In that case, the default is likely also an empty Array as it is consistent with other keys from options. Where do you think a user may have added the rescue? If they were overriding this method then this change would not affect them. 🤔 What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@amrani well right now if options doesn't have the classes key this will raise a KeyError, not sure about the whole architecture of the code so can't exactly tell you where that might have happened. Somebody with more codebase experience will probably chime in here :) But yea, rescues will be consistent with this new default probably.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not (off the top of my head) aware of anything which would catch this otherwise and I'd assume that that Dashboard would throw an exception.

@nickcharlton
Copy link
Member

Thanks for catching this and adding the tests!

@nickcharlton nickcharlton merged commit acaad1e into thoughtbot:master Apr 20, 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.

3 participants