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

Support for private callbacks #53

Merged
merged 4 commits into from
Sep 3, 2012
Merged

Support for private callbacks #53

merged 4 commits into from
Sep 3, 2012

Conversation

dwbutler
Copy link

I defined some callbacks on my model. The problem is that the methods look like the public API, and will cause confusion and bugs when users of the API call the callback methods directly instead of the workflow methods.

(For example, it would be easy to mistakenly call task.assign instead of task.assign!)

My first instinct was to make the callback methods private, but this didn't work. Digging deeper, I found out this is because workflow doesn't look for private callbacks. This patch helps workflow find private callbacks.

@geekq
Copy link
Owner

geekq commented Aug 27, 2012

I like the idea of incapsulation. Will merge in your improvement! I just wish more expressive unit test: instead of another_transition would name it some_private_transition_method in the test.

@dwbutler
Copy link
Author

I agree about the tests! They were a bit messy so I just hacked something in. I'll take some time to create a more descriptive test case for this.

@dwbutler
Copy link
Author

I added a separate test, and also fixed ruby 1.8.x compatibility.

@geekq geekq merged commit 7214ade into geekq:master Sep 3, 2012
@geekq
Copy link
Owner

geekq commented Sep 3, 2012

Merged and tested with Ruby 1.8.7 + Rails 2.2.3 and Ruby 1.9.3 + Rails 3.2.2.

Thanks for your patience! ;-)

@dwbutler
Copy link
Author

dwbutler commented Sep 4, 2012

Awesome, thanks!

@geekq
Copy link
Owner

geekq commented Jan 19, 2013

Now changed to "support for non public transition callbacks", see also #58

geekq added a commit that referenced this pull request Jan 20, 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.

2 participants