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

Allow presenters to define initialize via inheritance #56

Merged
merged 2 commits into from
May 14, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions lib/shortcode/presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@ def self.register(configuration, presenter)

def self.validate(presenter)
raise ArgumentError, "The presenter must define the class method #for" unless presenter.respond_to?(:for)
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)
Copy link
Owner

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

@lukebergen lukebergen May 4, 2019

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kernow any thoughts on this?

Copy link
Owner

Choose a reason for hiding this comment

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

@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!


%w[content attributes].each do |method|
unless presenter.method_defined?(method.to_sym)
Expand All @@ -18,6 +16,12 @@ def self.validate(presenter)
end
end

def self.init_defined?(presenter)
return false if presenter == Object

presenter.private_instance_methods(false).include?(:initialize) || init_defined?(presenter.superclass)
end

def initialize(name, configuration, attributes, content, additional_attributes)
@configuration = configuration
@attributes = attributes
Expand Down
16 changes: 16 additions & 0 deletions spec/shortcode/presenter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
require "support/presenters/multiple_presenter"
require "support/presenters/missing_for_presenter"
require "support/presenters/missing_initialize_presenter"
require "support/presenters/child_presenter"
require "support/presenters/child_missing_initialize_presenter"
require "support/presenters/missing_content_presenter"
require "support/presenters/missing_attributes_presenter"

Expand Down Expand Up @@ -101,6 +103,20 @@
end
end

context "when initialize exists on non-Object ancestor class" do
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@lukebergen lukebergen May 4, 2019

Choose a reason for hiding this comment

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

Good call. Will add one. edit: Added

it "does not raise an exception" do
expect { shortcode.register_presenter ChildPresenter }
.not_to raise_error
end
end

context "when self and ancestory are missing #initialize method" do
it "raises an exception" do
expect { shortcode.register_presenter ChildMissingInitializePresenter }
.to raise_error(ArgumentError, "The presenter must define an initialize method")
end
end

context "when missing #content method" do
it "raises an exception" do
expect { shortcode.register_presenter MissingContentPresenter }
Expand Down
11 changes: 11 additions & 0 deletions spec/support/presenters/child_missing_initialize_presenter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
require "support/presenters/parent_missing_initialize_presenter"

class ChildMissingInitializePresenter < ParentMissingInitializePresenter

def self.for; end

def content; end

def attributes; end

end
11 changes: 11 additions & 0 deletions spec/support/presenters/child_presenter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
require "support/presenters/parent_presenter"

class ChildPresenter < ParentPresenter

def self.for; end

def content; end

def attributes; end

end
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class ParentMissingInitializePresenter
end
5 changes: 5 additions & 0 deletions spec/support/presenters/parent_presenter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class ParentPresenter

def initialize; end

end