-
Notifications
You must be signed in to change notification settings - Fork 138
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
Allow the creation and use of custom actions #36
Allow the creation and use of custom actions #36
Conversation
include Ethon::Easy::Http::Actionable | ||
include Ethon::Easy::Http::Postable | ||
|
||
class << self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't this be an instance variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that Ethon::Easy::Http::Custom
should be instantiated with the verb we'd like it to use? That would work, but it would require that we special-case Ethon::Easy::Http::Custom
's initialization or we change initialization for all actions. I was trying to avoid that, but maybe it's worth changing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now I see why you did it that way. What do you think about adding the action_name to the options in http_request
? It would be available then to the object and there are no big changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, that would work as well. Let me make the changes quick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's turning out pretty ugly.
It looks great, thanks! |
Would you mind having another look at this? I think what I've arrived at is fairly nice. |
Thats awesome, thank you very much for your work! Will be merged! Could you squash your PR into one or two commits? |
I can, but I'll need to open another pull request since I've already pushed this to github. |
I guess it's unlikely anyone else has pulled from my repository, actually, so I can force push. |
Squashed, have at it. :) |
Allow the creation and use of custom actions
Addresses #35.