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

Context key aliases #68

Closed
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jpmoral
Copy link
Contributor

jpmoral commented Jul 4, 2015

This addresses #67. An aliases macro was added to the Organizer module. It accepts a hash; the keys are the original keys and the values are the key aliases.

This is passed to WithReducer#with. WithReducer#reduce inserts new key-value pairs into the context: the keys are the key aliases while the values are the values corresponding to the original keys. Thus each action can use the aliases when accessing the context. Example usage:

class AnOrganizer
  extend LightService::Organizer

  aliases my_key: :key_alias

  def self.for_order(order)
    with(:order => order).reduce(
        AnAction,
        AnotherAction,
      )
  end
end

class AnAction
  extend LightService::Action
  promises :my_key

  executed do |context|
    context.my_key = "value"
  end
end

class AnotherAction
  extend LightService::Action
  expects :key_alias

  executed do |context|
    context.key_alias # => "value"
  end
end

See spec/organizer_key_aliases_spec.rb for more.

jpmoral added some commits Jul 4, 2015

WithReducer accepts aliases
aliases is a hash.  Keys are keys found in the context, values are
aliases for the keys. For example, given the following:

data = { foo: 'foo' }
aliases = { foo: :alias_for_foo }

context passed to the actions will be:

{ foo: 'foo', alias_for_foo: 'foo' }

See spec/organizer/with_reducer_spec.rb
Add `aliases` macro to Organizer
aliases is passed to WithReducer to make key aliases available to all
actions

See spec/organizer_key_aliases_spec.rb

@jpmoral jpmoral changed the title Key aliases Context key aliases Jul 4, 2015

@adomokos

This comment has been minimized.

Copy link
Owner

adomokos commented Jul 6, 2015

@jpmoral: Thank you for working on this PR!

You have many good changes here that I'd like to have in LS. I looked at how you are setting the aliases in the WithReducer class. I was wondering if we could delegate that responsibility to the Context class.

Check out this commit. I tried to see how it would work, but the specs are not passing there.

What do you think?

@jpmoral

This comment has been minimized.

Copy link
Contributor

jpmoral commented on lib/light-service/organizer/with_reducer.rb in d4fad8b Jul 6, 2015

The problem is that the keys might not be in the context yet at this point. How about moving it to where put_aliases_in_the_context used to be?

This comment has been minimized.

Copy link
Owner Author

adomokos replied Jul 6, 2015

That could be a better alternative.

It feels to me that the aliases field is leaking through objects. Look at how many objects are aware of this change. I wish I had a better idea of how to do this, but I don't. I am just asking you if you could think of any other way to simplify the code. Thoughts?

This comment has been minimized.

Copy link
Contributor

jpmoral replied Jul 6, 2015

I am working on two options to present

  1. What I suggested above, call Context#set_aliases before each action execution.
  2. Have context automagically serve the right value. This involves overriding Hash#[].

I'll put links in the main PR discussion when I figure out how to make diff links. :)

@jpmoral

This comment has been minimized.

Copy link
Contributor Author

jpmoral commented Jul 6, 2015

@adomokos
I think it would be nice to have Context handle the aliases, and I did give it some thought while writing the code.
The main issue I faced was that we do not know beforehand when the keys to be aliased will be available in the context: whether the organizer or one of the actions provides them. That's why I put the aliasing in WithReducer. I admit put_aliases_in_the_context is an awkward name and is called in an awkward place.
I have two suggestions:

  1. Call Context#set_aliases before each action execution
  2. Give Context the aliases and have it automagically return the correct values. This cleans up WithReducer but I feel the code in Context is now a bit too clever.
@jpmoral

This comment has been minimized.

Copy link
Contributor Author

jpmoral commented Jul 6, 2015

@adomokos
Thoughts?

@adomokos

This comment has been minimized.

Copy link
Owner

adomokos commented Jul 7, 2015

@jpmoral: these are great suggestions, thank you for thinking about it!!

Here is what I hacked on tonight. It's not perfect as it still has one spec failing, but I think it clearly expresses what I'd like to see. Please note how I use the data to attach the _aliases key-value pair to it in the Organizer class. This way the data is a pass-through in WithReducer, that class is not aware of the aliases collection.

Now only the Organizer and the Context are the two entities or objects that are aware of the aliases hash. Could you get this one to work?

@jpmoral

This comment has been minimized.

Copy link
Contributor Author

jpmoral commented Jul 7, 2015

@adomokos
Please see the updated diff. I liked how WithReducer no longer knows about the aliases. If the changes are good there are a few more specs I'd like to add. Also, do you mind if I squash the new commits together? Some of them don't make sense as individual commits.

Edit:
Oops, there's still something wrong with it. Using dot notation (with expected and promised keys) does not yield the correct results.

@adomokos

This comment has been minimized.

Copy link
Owner

adomokos commented Jul 7, 2015

@jpmoral: I really like where you're going with this. Please squash the commits, add the specs and I'll merge in the code. Really nice work!! 👍

end
end
class FooAction

This comment has been minimized.

@adomokos

adomokos Jul 7, 2015

Owner

This should be AnotherAction in this example.

This comment has been minimized.

@jpmoral

jpmoral Jul 7, 2015

Author Contributor

Good catch!

@jpmoral jpmoral referenced this pull request Jul 8, 2015

Merged

Key aliases rebased #69

@jpmoral

This comment has been minimized.

Copy link
Contributor Author

jpmoral commented Jul 8, 2015

Closing this in favor of #69.

@jpmoral jpmoral closed this Jul 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.