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

When decoration fails, show original class #821

Merged
merged 1 commit into from
Oct 12, 2017
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
6 changes: 3 additions & 3 deletions lib/draper/decoratable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,16 @@ def decorator_class?
# `Product` maps to `ProductDecorator`).
#
# @return [Class] the inferred decorator class.
def decorator_class
def decorator_class(called_on = self)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pascalbetz Thanks for the contribution! I am not crazy about a public(ish) method taking a parameter that is only relevant to draper's internals. However, I can't think of a better way to accomplish this and it definitely solves a longstanding (and very annoying) problem.

Can you rebase with master before I merge? There has been some changes to this method in master as well and want to make sure the build passes. After that, I think we are good to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codebycliff I understand.
This was the easiest way to solve the problem... Alternatively we could change from recursion to iteration, this would get rid of the parameter. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pascalbetz It looks like however you merged, it brought in master changes to this pull request... Can you try again with rebase?

As far as the solution goes, I'm not sure. I'm not super opposed to this solution, however, I am interested in what the iteration solution looks like. Especially now that decorator_class returns nil if can't be found. It might make the iteration easier.

prefix = respond_to?(:model_name) ? model_name : name
decorator_name = "#{prefix}Decorator"
decorator_name_constant = decorator_name.safe_constantize
return decorator_name_constant unless decorator_name_constant.nil?

if superclass.respond_to?(:decorator_class)
superclass.decorator_class
superclass.decorator_class(called_on)
else
raise Draper::UninferrableDecoratorError.new(self)
raise Draper::UninferrableDecoratorError.new(called_on)
end
end

Expand Down
10 changes: 10 additions & 0 deletions spec/draper/decoratable_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,16 @@ module Draper
expect(Product).to receive(:decorator_class).and_return(:some_decorator)
expect(product.decorator_class).to be :some_decorator
end

it "specifies the class that #decorator_class was first called on (superclass)" do
person = Person.new
expect { person.decorator_class }.to raise_error(Draper::UninferrableDecoratorError, 'Could not infer a decorator for Person.')
end

it "specifies the class that #decorator_class was first called on (subclass)" do
child = Child.new
expect { child.decorator_class }.to raise_error(Draper::UninferrableDecoratorError, 'Could not infer a decorator for Child.')
end
end

describe "#==" do
Expand Down
2 changes: 2 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ class Model; include Draper::Decoratable; end
class Product < Model; end
class SpecialProduct < Product; end
class Other < Model; end
class Person < Model; end
class Child < Person; end
class ProductDecorator < Draper::Decorator; end
class ProductsDecorator < Draper::CollectionDecorator; end

Expand Down