Allow presenters to define initialize via inheritance #56
Conversation
lukebergen
force-pushed
the
presenter-inheritance-support
branch
2 times, most recently
from
1dc742a
to
ef44738
Compare
|
@kernow Hmm, any idea what might be wrong with rubocop/bundler on Travis? I ran it locally and got 0 errors. The errors in Travis seem to be unrelated to my code changes here. Thoughts? |
|
@lukebergen firstly thanks for the PR! The errors look like they're unrelated to your changes. I'll see if I can make some time this week to fix the bundler version issue. |
|
@lukebergen I've updated master to fix this issue. The ruby and rails versions its tested against were rather out of date. You should be able to remove you rubocop commit and rebase now |
It would be nice to be able to define a BasePresenter that defines some base helper methods or even defines a default behavior for the 4 required methods. Then it would be up to any child classes to override that default behavior. Currently this is possible for `self.for`, `content`, and `attributes`. But initialize is weird because `Object` defines initialize and we don't want that to "count". So in order to support the initialize being inheritable to child classes, I've changed the condition around when we throw the validation error to check the presenter's class ancestory _until_ we reach Object. At that point we assume failure and raise the validation error.
lukebergen
force-pushed
the
presenter-inheritance-support
branch
from
ef44738
to
7e84924
Compare
|
@kernow cool. Backed out the rubocop commit so now it's just my new code + tests. Everything's green! Thanks for the help. |
| unless presenter.private_instance_methods(false).include?(:initialize) | ||
| raise ArgumentError, "The presenter must define an initialize method" | ||
| end | ||
| raise ArgumentError, "The presenter must define an initialize method" unless init_defined?(presenter) |
There was a problem hiding this comment.
@lukebergen you should be able to use presenter.private_method_defined?(: initialize) which takes into account super class methods. See: http://ruby-doc.org/core-2.6.3/Module.html#method-i-private_method_defined-3F
There was a problem hiding this comment.
@kernow Based on that documentation, it looks like that'd return false unless the initialize was defined as a private method. Which I don't think we'd want?
I wrote it the way I did because I don't want Object's initialize to "count" but do want to allow for Object > GrandparentPresenter > ParentPresenter > ..... > VerySpecificPresenter to allow for anybody up the ancestor chain to define initialize BUT as soon as we hit Object, we know that initialize shouldn't be considered a legitimate Presenter initialize.
I didn't dig too deep into the libraries core though so I may be missing something.
There was a problem hiding this comment.
@kernow any thoughts on this?
There was a problem hiding this comment.
@lukebergen I was away for most of last week, just catching up now 🙂, I expected that private_method_defined? would not include Object but looks like thats not that case. Your solution looks good in that case. I'll get it merged and a gem released in the next couple of days. Thanks for your efforts!
| @@ -101,6 +102,13 @@ | |||
| end | |||
| end | |||
|
|
|||
| context "when initialize exists on non-Object ancestor class" do | |||
There was a problem hiding this comment.
Would be good to have an unhappy path test as well that checks for an exception when neither a parent or child class define the initialize method.
There was a problem hiding this comment.
Good call. Will add one. edit: Added
It would be nice to be able to define a BasePresenter that defines some
base helper methods or even defines a default behavior for the 4
required methods. Then it would be up to any child classes to override
that default behavior.
Currently this is possible for
self.for,content, andattributes.But initialize is weird because
Objectdefines initialize and we don'twant that to "count".
So in order to support the initialize being inheritable to child
classes, I've changed the condition around when we throw the validation
error to check the presenter's class ancestory until we reach Object.
At that point we assume failure and raise the validation error.