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 #decorated? method to Draper::CollectionDecorator #447

Closed
carloslopes opened this issue Jan 31, 2013 · 4 comments · Fixed by #460
Closed

Add #decorated? method to Draper::CollectionDecorator #447

carloslopes opened this issue Jan 31, 2013 · 4 comments · Fixed by #460

Comments

@carloslopes
Copy link
Contributor

Hi,

when i want to check if an object is decorated i call #decorated? and it will inform me. This is really useful when testing.

But when i have a Draper::CollectionDecorator i can't do this.

What do you think about add the #decorated? method on collections too?

If you think this useful, i can try to make it works.

@haines
Copy link
Contributor

haines commented Jan 31, 2013

I do like as many things as possible to be similar between the collection decorators and the regular decorators!

The one problem is that for a decorator, we can have decorated? return true, and for anything that includes Decoratable (ie models) decorated? returns false, so it works quite nicely:

model.decorated? # => false
Decorator.new(model).decorated? # => true

Unfortunately for a collection decorator, we don't have a way of implementing a decorated? method for the source object.

[].decorated? # => NoMethodError
CollectionDecorator.new([]).decorated? # => true

I don't think the method is as useful without its counterpart. However, I'm not completely opposed to adding it, because if you write a spec like

expect(products).to be_decorated

it will still fail when the subject is not decorated, although less gracefully!

@carloslopes
Copy link
Contributor Author

hm yes.

i think that in this case it will not be a problem if the source object doesn't have the #decorated? method, because this will be more useful for tests.

Does this have a chance to be merged if i do it?

@haines
Copy link
Contributor

haines commented Jan 31, 2013

I would give it a 👍 - @steveklabnik?

@steveklabnik
Copy link
Member

Yeah, seems fine to me. The test case is compelling.

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 a pull request may close this issue.

3 participants