Restore context #385

Merged
merged 4 commits into from Dec 13, 2012

Conversation

Projects
None yet
4 participants
Contributor

haines commented Dec 13, 2012

Includes #384 but after rebasing master it wouldn't merge cleanly back into @tovodeverett's branch, sorry! One less merge commit this way I guess.

I just changed it so it wasn't storing context in the options hash unnecessarily, which was one of my main objections with the original implementation that I removed.

I also came up with a neater way to avoid forcing enumeration in CollectionDecorator when changing the context.

I'd still like to refactor DecoratedAssociation#decorator_options but it's all in the private interface so it's not a blocker.

Contributor

tovodeverett commented Dec 13, 2012

I like the extraction of all options (including context) in Decorator#initialize and CollectionDecorator#initialize, while retaining options as an ivar in DecoratedAssociation. I also really like using @decorated_collection as cleaner way to avoid forcing enumeration - I think that was my initial plan several days ago when I was looking at the code (I need to start making more notes), but when I got to implementation I latched on to loaded? for some reason.

@steveklabnik steveklabnik added a commit that referenced this pull request Dec 13, 2012

@steveklabnik steveklabnik Merge pull request #385 from haines/context
Restore context
9609156

@steveklabnik steveklabnik merged commit 9609156 into drapergem:master Dec 13, 2012

1 check passed

default The Travis build passed
Details
Owner

steveklabnik commented Dec 13, 2012

Thank you @haines ! Also, with this merge, I did something that I think is long overdue: welcome to Team Draper! ❤️ You've been making a lot of badass contributions lately.

Contributor

haines commented Dec 13, 2012

Cool! Thanks Steve!! 💖

Contributor

tovodeverett commented Dec 13, 2012

One thing just occurred to me. This is a interface change. Should we update the Changelog and declare this as 1.0.0.beta4 so as to make it a little more obvious to anyone who might be using the beta code?

Owner

steveklabnik commented Dec 13, 2012

There should be a CHANGELOG entry for sure, and I plan on knocking out a beta4 as soon as we're back to zero issues. Lately, I've just been making CHANGELOGs right before release, but I want to change that with 1.0.

Contributor

tovodeverett commented Dec 13, 2012

Oh, and congrats to @haines 👏

I'd been wondering why he wasn't officially on the team, since it was evident that he was effectively on it!

elado commented Dec 14, 2012

This feature eliminates the option to pass custom options to decorators.

I used it a couple of times, ProductDecorator.new(p, my_option: true) and now it says "Unknown key: my_option".

Is the validation really necessary? The custom options can be really useful.

Or is it now context's job to pass options

Contributor

tovodeverett commented Dec 14, 2012

There was a lot of discussion on this - this change reverts the behavior back to the way it existed prior to aeac106. Yes, it does require code changes, but after some debating back and forth it was decided that it was better to get the interface changes done before the code exits beta.

You'll need to modify code that looks like this:

decorator = model.decorate(current_user: current_user)
decorator.options[:current_user]

To be rewritten to look like this:

decorator = model.decorate(context: {current_user: current_user})
decorator.context[:current_user]

The benefits are:

  • We can add options in the future without worrying about colliding with keys in the context
  • There are no longer off-limits keys in the context for decorated collections (i.e. :with and :scope)
  • Child objects of decorated associations automatically inherit their parent's context unless the decorated association explicitly sets the context. Passing a lambda in for the context allows the decorated association to get a dynamically generated context that gets the parent's context as input (i.e. you can filter context before handing it off to the association).
Contributor

haines commented Dec 14, 2012

@elado Yep, now you should write ProductDecorator.new(p, context: {my_option: true}) and access it via the context method instead of options. It's a bit longer, unfortunately, but, as @tovodeverett points out, there are a number of benefits.

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