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

Callback symbols and transition callbacks by convention #241

Closed
wants to merge 4 commits into from

Conversation

tlloydthwaites
Copy link

When building a workflow spec dynamically we can't specify procs, but we can specify symbols for methods we want to run.

This PR allows us to specify a symbol when calling on_entry and on_exit.

It also tries to call before_transition, on_transition, after_transition, and on_error as methods on the workflow object as well as using the standard procs.

@tlloydthwaites tlloydthwaites marked this pull request as ready for review March 7, 2024 04:05
@geekq geekq force-pushed the develop branch 3 times, most recently from b97ab82 to 970f839 Compare May 17, 2024 07:59
geekq added a commit that referenced this pull request May 29, 2024
@geekq
Copy link
Owner

geekq commented May 29, 2024

Thanks for your contribution!
This pull request contains two unrelated things:

  1. For general hooks like before_transition, on_error, on_entry etc. in addition to blocks also support methods by naming convention
  2. Allow symbols instead of specifing procs when defining workflow from some hash programmatically

For the first one I have no objections except minor incompatibility issue in case somebody defined e.g. an on_error method to do something unrelated to workflow. Formally we would need to wait until a major 4.0 release and add some deprecation warning already. Or maybe use more specific method names like on_workflow_error instead of on_error, where it is more unlikely for somebody to already have such a method for a different purpose. After carefully considering pros and contra, I would stick with existing functionality for now.

For the second part, I think it brings a lot of additional complexity to the very core of workflow by storing symbols instead of Procs sometimes.

Dynamically composing workflow definition from a json file, could be useful for some people though. Thank you for providing an extensive example! The desired result can be achieved with existing features of workflow, without changes to the library like that: https://github.com/geekq/workflow/blob/develop/test/workflow_from_json_test.rb
I started with your original test file and made some minor adjustments to take advantage of the existing workflow model.

@geekq geekq closed this May 29, 2024
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

2 participants