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

Assign expected and promised keys to Action subclass #72

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@jpmoral
Copy link
Contributor

jpmoral commented Jul 18, 2015

Currently when an Action is subclassed the subclass cannot access the expected and promised keys using dot notation.

class AnAction
  extend LightService::Action
  expects :expected_key
  executed do |context|
    # do stuff
  end
end

class AnActionSubclass
  executed do |context|
    # do stuff
    context.expected_key # raises an error
  end
end

This PR will allow the above to work.

@jpmoral

This comment has been minimized.

Copy link
Contributor Author

jpmoral commented Jul 18, 2015

@adomokos

I discovered this a couple of weeks ago. I was able to eliminate the need for subclassing by going with a different (cleaner) design of Action and Context hash so I'm not sure how useful this is.

However, I made the PR because it might be surprising to others that expects and promises don't work with inheritance.

@adomokos

This comment has been minimized.

Copy link
Owner

adomokos commented Sep 22, 2015

@jpmoral, thank you for submitting this PR.

The reason I opted to use class methods for the actions was to limit inheritance. With inheritance there is an "inferred" state that I tried to avoid.

Would you be OK if I did not merge this PR? I know you worked on it, that's why I feel bad about it.

@jpmoral

This comment has been minimized.

Copy link
Contributor Author

jpmoral commented Sep 22, 2015

@adomokos

It's no problem, like I mentioned, even I'm a bit iffy about it. It might be helpful though to be explicit in the docs that inheritance of actions isn't really encouraged.

@adomokos

This comment has been minimized.

Copy link
Owner

adomokos commented Sep 22, 2015

Good suggestion!

@adomokos adomokos closed this Sep 22, 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.