Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix inheriting from a base class that does not define any attributes. #7

Merged
merged 1 commit into from
Mar 15, 2017
Merged

Fix inheriting from a base class that does not define any attributes. #7

merged 1 commit into from
Mar 15, 2017

Conversation

tatey
Copy link
Contributor

@tatey tatey commented Mar 15, 2017

Hello,

Firstly, thank you for creating this project 😄. We are using it for our non-ActiveRecord backed forms.

We have a base class that mixes in ShallowAttributes, but doesn't define attributes of its own. Any engineer can subclass this base class and have confidence that it will work out of the box in our forms and controllers.

Unfortunately we hit a bug where the subclass would raise TypeError: no implicit conversion of nil into Hash because it's trying to read default_values from a nil object which it expected to be a hash.

We have managed to work around it by defining MyExampleBaseClass.default_values to return an empty hash. While this is satisfactory it means the out of box experience with ShallowAttributes is not as smooth as it could be.

I hope you find this patch valuable and I'm open to any feedback you have.

Cheers,
Tate

#
def inherited(subclass)
super
if respond_to?(:default_values)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given ShallowAttributes is intended to be a mixin we can't be certain that the base class will implement default_values. That's why it's necessary for us to check first.

@@ -66,8 +74,7 @@ def attributes
def attribute(name, type, options = {})
options[:default] ||= [] if type == Array

@default_values ||= {}
@default_values[name] = options.delete(:default)
default_values[name] = options.delete(:default)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As an added benefit to using the injection hook default_values is now a getter that always returns a hash.

@coveralls
Copy link

coveralls commented Mar 15, 2017

Coverage Status

Coverage increased (+0.02%) to 99.077% when pulling 9900e85 on fivegoodfriends:fix-inheritance-when-base-class-has-no-attributes into dd82ec6 on davydovanton:master.

@davydovanton
Copy link
Owner

Hey,
I'm happy that for someone this gem is useful 😊
Changes and motivation look good, that's why I'll merge it. Thanks for contribution 👍

@davydovanton davydovanton merged commit 3740a54 into davydovanton:master Mar 15, 2017
@tatey
Copy link
Contributor Author

tatey commented Mar 15, 2017

Glad you like it 👍

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants