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

Change Decorator attribute 'source' to 'object' #501

Closed
johnnyshields opened this Issue Mar 19, 2013 · 12 comments

Comments

Projects
None yet
5 participants
@johnnyshields

johnnyshields commented Mar 19, 2013

'source' is a very common word, and in my app I have cases where I want to use it as a database field. I could use 'origin', 'channel', or 'src' but not as understandable.

I would propose to change the Decorator attribute to 'source_instance', and make 'source' as an alias.

I would also propose that delegate_all (AutomaticDelegation) should check for the presence of methods source, to_source, and model on the base class, and if they are present pass-thru to them instead of the Decorator aliases for source_instance.

If there's agreement to do this, I'm glad to contribute the code.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik
Member

steveklabnik commented Mar 19, 2013

Hmm.

@haines

This comment has been minimized.

Show comment
Hide comment
@haines

haines Mar 19, 2013

Contributor

Yep, I agree with your first proposal. Perhaps let's just call it _source though, since that is a fairly common pattern in Rails for 'private' methods.

However, I don't think your second suggestion makes sense; delegate_all uses method_missing, so it won't get hit for model and friends because they are already defined explicitly on the decorator.

Contributor

haines commented Mar 19, 2013

Yep, I agree with your first proposal. Perhaps let's just call it _source though, since that is a fairly common pattern in Rails for 'private' methods.

However, I don't think your second suggestion makes sense; delegate_all uses method_missing, so it won't get hit for model and friends because they are already defined explicitly on the decorator.

@jerodsanto

This comment has been minimized.

Show comment
Hide comment
@jerodsanto

jerodsanto Mar 19, 2013

I believe ActiveModel::Serializers uses object to refer to the thing being serialized. That makes just as much sense in terms of a decorator as source does and it probably has less name collisions with db fields.

Worth a consideration, at least.

jerodsanto commented Mar 19, 2013

I believe ActiveModel::Serializers uses object to refer to the thing being serialized. That makes just as much sense in terms of a decorator as source does and it probably has less name collisions with db fields.

Worth a consideration, at least.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Mar 19, 2013

Member

Yeah, I like object.

Member

steveklabnik commented Mar 19, 2013

Yeah, I like object.

@haines

This comment has been minimized.

Show comment
Hide comment
@haines

haines Mar 19, 2013

Contributor

I like object for Decorator, but what about CollectionDecorator? I think it's worthwhile keeping them the same, but I don't think object works for both.

Contributor

haines commented Mar 19, 2013

I like object for Decorator, but what about CollectionDecorator? I think it's worthwhile keeping them the same, but I don't think object works for both.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Mar 19, 2013

Member

It is a little awkward, but it might be the least bad option.

Member

steveklabnik commented Mar 19, 2013

It is a little awkward, but it might be the least bad option.

@johnnyshields

This comment has been minimized.

Show comment
Hide comment
@johnnyshields

johnnyshields Mar 20, 2013

I think object or objects would work for CollectionDecorator--seems as understandable as source.

Are we prefixing object with underscore? (_object).

Also, do we really need the aliases source, model, and to_source. Maybe these can be deprecated?

johnnyshields commented Mar 20, 2013

I think object or objects would work for CollectionDecorator--seems as understandable as source.

Are we prefixing object with underscore? (_object).

Also, do we really need the aliases source, model, and to_source. Maybe these can be deprecated?

@haines

This comment has been minimized.

Show comment
Hide comment
@haines

haines Mar 20, 2013

Contributor

Ok, I'm sold, let's use object for both (Draper::Delegation needs them to be the same), and no prefix - AM::Serializers doesn't use one either.

I think we should worry about deprecation a little closer to 2.0, as we won't be removing anything before then anyway.

@johnnyshields are you still keen to code this up?

Contributor

haines commented Mar 20, 2013

Ok, I'm sold, let's use object for both (Draper::Delegation needs them to be the same), and no prefix - AM::Serializers doesn't use one either.

I think we should worry about deprecation a little closer to 2.0, as we won't be removing anything before then anyway.

@johnnyshields are you still keen to code this up?

@johnnyshields

This comment has been minimized.

Show comment
Hide comment
@johnnyshields

johnnyshields Mar 20, 2013

Roger that, will use object with no underscore.

johnnyshields commented Mar 20, 2013

Roger that, will use object with no underscore.

@johnnyshields

This comment has been minimized.

Show comment
Hide comment
@johnnyshields

johnnyshields Apr 2, 2013

Hi guys, this is still on my radar. Have a product to ship at work first, then I'm on this.

johnnyshields commented Apr 2, 2013

Hi guys, this is still on my radar. Have a product to ship at work first, then I'm on this.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Apr 2, 2013

Member

Sounds good. :)

Member

steveklabnik commented Apr 2, 2013

Sounds good. :)

@denishaskin

This comment has been minimized.

Show comment
Hide comment
@denishaskin

denishaskin Dec 28, 2014

I don't believe this issue should have been closed; the original issue hasn't really been addressed. Commit 4b933ef changes the attribute name to object and changes all internal references, but leaves source as an alias, so for those of us where the problem was that our model already has a source attribute... this is still an open issue.

There is a :TODO: in the source code for this to be addressed - https://github.com/drapergem/draper/blob/master/lib/draper/decorator.rb#L13 - but it took a little hunting.

denishaskin commented Dec 28, 2014

I don't believe this issue should have been closed; the original issue hasn't really been addressed. Commit 4b933ef changes the attribute name to object and changes all internal references, but leaves source as an alias, so for those of us where the problem was that our model already has a source attribute... this is still an open issue.

There is a :TODO: in the source code for this to be addressed - https://github.com/drapergem/draper/blob/master/lib/draper/decorator.rb#L13 - but it took a little hunting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment