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

Idiomatic callbacks #91

Closed
wants to merge 6 commits into from
Closed

Idiomatic callbacks #91

wants to merge 6 commits into from

Conversation

jamiehodge
Copy link

This is certainly a case of pickiness, but is_authorized? and is_conflict? don't feel very idiomatic. I've renamed them to authorized? and conflict?, so that they more closely resemble, for example, forbidden?. I was also tempted to rename generate_etag to etag, but saw that a number of tests currently include :etag.

I realize that there would need to be a deprecation of the existing syntax, but I a) didn't know if you already have a deprecation policy or; b) whether you agreed with the changes to begin with.

Regards,

Jamie

@ghost
Copy link

ghost commented Jan 5, 2013

Agree on the idiomacy, but as we're past 1.0, we need to keep the is_ versions around. An alias authorized? => is_authorized? would be enough for that, and would give us time to consider how deprecations could work.

@jamiehodge
Copy link
Author

I'll add the aliases.

What are your feelings regarding renaming generate_etag to etag? I understand the callbacks as attributes of the resource, which makes the calculation prefix inappropriate IMHO.

Jamie

@jamiehodge
Copy link
Author

I've added aliases for authorized?, conflict? and etag for backwards compatibility. There don't appear to be any tests for the callbacks specifically, so there are no accompanying tests. Perhaps another tasks...

@ghost
Copy link

ghost commented Jan 5, 2013

There are specs for Webmachine::Decision::Flow, which is used for gathering facts about a resource. I guess it's best to just grep the codebase for the methods names you changed.

First interesting question about changing the resource API: in the transition phase, do we test the old or new methods? 😈

@jamiehodge
Copy link
Author

I have changed all the specs to use the new methods. I hope this is acceptable.

On 05/01/2013, at 22.00, Lars Gierth notifications@github.com wrote:

There are specs for Webmachine::Decision::Flow, which is used for gathering facts about a resource. I guess it's best to just grep the codebase for the methods names you changed.

First interesting question about changing the resource API: in the transition phase, do we test the old or new methods?


Reply to this email directly or view it on GitHub.

@bernd
Copy link
Contributor

bernd commented Jan 6, 2013

I guess @seancribbs wanted to keep the callback names close to the ones in basho/webmachine. If that is still a goal, I would oppose to this change to keep the callback API small.

@jamiehodge
Copy link
Author

Let's wait for his reaction. I don't see any additional changes, so I would argue that three aliases is not egregious.

@ghost
Copy link

ghost commented Jan 6, 2013

I agree that aliases are totally fine, even if I don't like all of the proposed changes (i probably prefer 'generate_etag' over just 'etag'), but having that choice is cool.

@seancribbs
Copy link
Member

This is stale, closing. I still maintain that there is value in keeping the callback names as close to the Erlang version, even if they aren't "idiomatic" to Ruby.

@seancribbs seancribbs closed this Jul 18, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants