Closed tovodeverett closed 11 years ago
What about setting it up so that if the user doesn't specify otherwise, decorates_association sets things up to automatically pass the parents' options hash to all of the children Decorators.
This would match what I'd expect too.
It's probably too much interface churn
We're preparing for a 1.0.0 release, 'churn' isn't a problem. Draper isn't a big library.
My worries about churn were more about the external interface. I wasn't sure how many people are depending upon 1.0.0 beta code, but I'm coming to the realization that one of the values of the Ruby community (and one I like) is permitting backwards incompatible changes for good reasons when going between versions (i.e. 1.8 -> 1.9, Rails 2->3, etc.). Thus ~>
.
So, since we're not that worried about interface churn, I'd definitely vote for renaming Draper::Decorator#options
- options has seems to have a pretty standard connotation within the Ruby/Rails community, and that seems to be as a way of passing parameters by name rather than by position. Since the options in the initializer aren't for the initializer, but rather for all the other methods, this is a bit of a stretch on the naming convention.
I will admit that even from the beginning I felt a little uncomfortable with the naming for Draper::Decorator#options
. I sort of like the old name, context
, but it has issues as well because it's too easy to conflate it with ViewContext
. I've been looking at a thesaurus, but that isn't really helping.
I need to do some thinking about the real purpose of the hash. Currently I'm using it to pass along some "collaborator" objects (i.e. current_user
and current_prefs
) as well as some user-supplied parameters from params
. In the latter case, I'm slicing params
first (to ensure I only pass what is relevant) and calling it img_params
(in this case, it's options for displaying a photograph, like how to resize it, whether to adjust the gamma, etc.) All of these could be passed directly in each method call, but that reduces DRYness. I keep thinking the whole thing feels vaguely like DCI for some reason. I'm going to go ruminate.
I wasn't sure how many people are depending upon 1.0.0 beta code,
Shouldn't be many, but SemVer makes no guarantees pre-1.0, and you shouldn't expect interfaces to be the same within betas.
I plan on following strict SemVer for Draper, so 1.0.0 will have a supported interface for a while, which is why getting it right now is so important.
I'm still thinking about the right name for Draper::Decorator#options
, but I have finally figured out that my initial impression about the options
parameter to Draper::DecoratedAssociation#initialize
was correct! It is indeed a dual-purpose hash - options like :with
are extracted from it before using it as the default for @options
which is indeed passed to the underlying decorator constructors. It looks like the :scope
option gets used but not extracted, so if you define :scope
, then it appears that the underlying decorators will get :scope
in their options
hash!
I think this reinforces the need to come up with a new name and to disassociate these two uses.
Also, using options=
to set the options on a decorated association takes a little care. Because DecoratedAssociation#call
returns the undecorated association if undecorated == []
, one can't call decorator.association.options =
without first testing to see if the association is empty. This also means that if I do it in initialize
, I'm forcing the enumeration of the association even if my code never touches it.
What about frame
? I vetoed setting
because it has specific alternate connotations, but another possibility might be stage
. There are two meanings of stage that would make sense in this situation - both the location for a performance and to assemble supplies for later use.
I'm not particularly opposed to options
, but context
would be ok too (not quite the old way of doing it, context
used to be a special-cased key in the options
hash). I don't think it's too easily conflated, you don't have to deal with view contexts too often and when you do it's called view_context
, right?
I'm happy with context
if everyone else is - I just assumed that options
got used because there were issues with calling it context
. Is there consensus on having context
get automatically passed through decorates_association
and if the user needs control over what gets passed, they can pass a block to decorates_association
that will take the parent context
as a parameter and will return the context
hash for the association. I don't see any advantage to statically configuring the context
hash for the association during the call to decorates_association
(and if someone does want that, they can easily do it in the block).
Also, I'm not sure I understand why DecoratedAssociation#call
returns the undecorated association if undecorated == []
. It was definitely unexpected, it makes testing potentially trickier, and it makes using options=
on the association dangerous if the user doesn't understand this corner case.
I'm not sure exactly (I only left []
undecorated because that was how it worked before), but I think the reasoning was similar to that of not decorating nil
.
However I don't think that makes sense for a CollectionDecorator
because although no instances will get individually decorated, it is probably desirable to have the pagination methods or whatever one might add to a custom ProductsDecorator
available even on an empty array.
@steveklabnik How do you feel about allowing []
to be decorated? I think it is still probably best to leave nil
undecorated though.
I don't have an opinion either way. I think there was some reason we didn't decorate []
...
Found it ... 5dd5757b25e77f262d3ca794952f6e3541824aaa
The problem was that we were getting the decorator class from orig_association.first.class
, which is of course nil
for an empty array. This is no longer the case, so it will work fine. I'll submit a pull request.
The original point of this thread was to discuss whether using options
for two different purposes (passing some sort of context to a decorator as well as for configuring a decorated association) was a good thing and to discuss whether there should be easier ways to configure options
(or whatever we decide to call it) for the decorated objects in a decorated association. Along the way I stumbled into the DecoratedAssociations#call
issue. The latter got resolved, but the first one is still out there.
Ahh, good point.
I'd enjoy taking a stab at implementation, but it seems like it would be good to get consensus on the desired behavior first. Here's my proposal:
:with
and :scope
options in decorates_association
, context
would replace options
for Decorator
and CollectionDecorator
.Decorator.decorates_association
would take a block that will be used to transform the parent object's context into the child objects' context. If the block is not passed, context will simply be passed along. The transforming of the parent object's context would happen in the call to DecoratedAssociation#decorate
(i.e. at the time the CollectionDecorator
or child object is instantiated). I think it's going to have to look at collection?
because behavior is going to have to differ between collections (which need the additional options in addition to the context) and singular objects (which just need the context).CollectionDecorator#context=
should set the context for the CollectionDecorator
the same way it's currently handled in #options=
, although I'd like to get the code to not force evaluation of the association if that has yet to happen.context
on the parent object gets modified after the association has been used, but the more I think about that the more I think that since handling it 100% robustly is rather difficult, it's best to say that's an unsupported scenario. I had everything worked out to handle someone calling #context=
on the parent object, but then I remembered that someone might also do parent.context[:foo] = 'bar'
and I wasn't about to get into trying to hook an observer on that hash! If there is no transformer block, there shouldn't be an issue because everyone will be sharing the same hash, but if there is a transformer it gets ugly.Should I go for it?
Sounds pretty good to me - except I think you mean :scope
not :source
;) I think the best thing would be to slice the hash passed to the initializer and have an options
hash (only keys :with
and :scope
) and a context
hash (everything else), and merge the two in decorate
. We need the options for singular associations too, so there should be no need to switch on collection?
So by default the associated object gets the parent context, optionally modified by a block. If I provide some context, e.g. decorates_association :foo, some: "data"
, what happens? If I give it a block as well, should the block just never be called? Perhaps even it should be an ArgumentError
to pass a block unless context
is empty?
On your fourth point, my response is the same as for #378, decorators should generally be write-once, so I wouldn't worry about that.
You are correct on :scope
not :source
(luckily I can modify the original comment).
The options don't make sense for singular associations once the association is instantiated. The :scope
option is used in DecoratedAssociation#undecorated
as part of creating the underlying association before decorating it - once that has happened, even CollectionDecorator
doesn't use it. The :with
option is used to determine how the individual objects in the collection (in the case of a collection) should be decorated (so CollectionDecorator
does indeed need it), but for singular associations the purpose of :with
is over once the singular object gets decorated in DecoratedAssociation#decorated
. The divergence in interface would only affect CollectionDecorator
(which is internal) and decorate_collection
. I'll think about this a bit, but the reason I'm hesitant to slice a merged hash is that we're making certain context keys "special" - for instance, if I have a Rifle model and the user has a default Scope that I pass in as part of the context (I know this is contrived), I may get a tad surprised. I don't think :with
and :scope
are likely to be that common, but what happens if we want to add another option to decorates_association
down the road - that suddenly becomes a SemVer major version change instead of a minor version.
Addressing your second paragraph, my suggestion would be that the hash passed to decorates_association
be purely for options. If a user wants to pass static context, the supported approach should be decorates_association :foo {|context| {some: "data"} }
. I suspect static context is going to be less common than dynamic context - dynamic context exists whenever the behavior of the decorator needs to change based on request-specific information, whereas static context for a decorated association would only exist when the behavior of the decorator needs to change because it is a child of a parent decorator of a given class.
Yep, that's a good point about having context and options in the same hash. In that case I think we should go back to passing context as a key to the various options hashes. That would mean something like
# Decorator and CollectionDecorator
def initialize(source, options = {})
# ...
@context = options.delete(:context) { {} }
end
The valid options to the various methods would be
decorate
: [:context]
decorate_collection
: [:context, :with]
decorates_association
: [:context, :with, :scope]
And passing :context
and a block to decorates_association
would be an ArgumentError
. I'm not keen on the block being the only way to set context, because if we have a non-block-based way of doing it in the other methods it should work for decorates_association
too. I'm not sure how people will use the feature and as such would prefer to leave the decision of static or dynamic context to the user - even if one or the other is more common, it's not hard to support them both.
PS CollectionDecorator
is not internal, it's totally legit to do class ProductsDecorator < Draper::CollectionDecorator
I won't argue - even though it tightens up the syntax when calling decorate
to not have to say decorate(foo: 'bar')
instead of decorate(context: {foo:
bar})
, I think the flexibility down the road is worth the extra characters.
What about decorates_association(context: lambda {|context| . . . })
- basically, if context responds to :call
then treat it as a block?
I agree that decorate(foo: "bar")
is neater (which is why I scrapped context in the first place), but you're right, it's better to be flexible (and consistent).
if context responds to :call then treat it as a block?
Yep, I like it!
Should I implement strict options (i.e. raise errors if invalid option keys are passed) or leave it as is (silently ignore)? I'm happy with the existing behavior.
I think silently ignoring them is fine.
Personally I prefer when options are strict / raise errors.
Jeff Casimir Principal, Jumpstart Lab http://jumpstartlab.com
On Wed, Dec 12, 2012 at 1:39 PM, Andrew Haines notifications@github.comwrote:
I think silently ignoring them is fine.
— Reply to this email directly or view it on GitHubhttps://github.com/drapergem/draper/issues/372#issuecomment-11302009.
@jcasimir Fair enough. I guess that would help stop people using options like :polymorphic
that were valid in 0.x but not in 1.0.
The one downside/upside is that it will force people who started using options
in place of context
to rapidly convert back to context
!
This is fixed with the merging of @haines' latest code, yes?
Yep, credit mostly due to @tovodeverett though!
Absolutely true. :heart: @tovodeverett !
It seems like the
options
parameter inDraper::Decorator#initialize
and inDraper::DecoratedAssociation#initialize
mean different things.In
Draper::Decorator#initialize
, theoptions
parameter exists solely to pass a list of things with which to populate#options
- i.e. contextual information of use to Decorator instances.In
Draper::DecoratedAssociation#initialize
, theoptions
parameter is a set of options for configuring theDecoratedAssociation
instance (i.e.:scope
and:with
). For a while, I thought that it actually conflated the first and the second. I just finally realized, however, that I have to call#options=
on the association (i.e.decorator.association.options = { option_for_child_decorator: 'foo' }
).What about setting it up so that if the user doesn't specify otherwise,
decorates_association
sets things up to automatically pass the parents'options
hash to all of the children Decorators. If the user wants control, the user should pass a block todecorates_association
that is responsible for generating theoptions
hash, like so:I suggest that some sort of naming change be at least considered. It's probably too much interface churn to rename
Draper::Decorator#options
, so my suggestion would be thatDraper::DecoratedAssociation#options
get renamed to something like#association_options
to distinguish it and reduce confusion.