geekq / workflow

Ruby finite-state-machine-inspired API for modeling workflow
MIT License
1.75k stars 207 forks source link

Add mongoid support #99

Closed bowsersenior closed 10 years ago

bowsersenior commented 10 years ago

This came up a few years ago (see #16 and #18) but I thought I'd try again as I see workflow now has separate adapter classes, which makes it a bit easier to add new adapters.

It's actually a bit of a pain to keep workflow_on_mongoid up to date with changes in workflow (see https://github.com/bowsersenior/workflow_on_mongoid/pull/12), so I thought it would be logical to add the small Mongoid adapter to workflow itself.

Let me know your thoughts. Thanks!

dwbutler commented 10 years ago

:+1:

geekq commented 10 years ago

I know, mongodb is pretty popular nowadays. So there is a good reason to support it.

On the other side including a mongodb adapter also means additional maintenance effort (even if workflow_on_mongoid contains an extended test suit) and maintenance obligation - once included into workflow library, if new issues with mongodb integration appear, that I can not debug, I can not just kick the adapter out again.

So for me the next step would be to check, why it so painful to keep up with workflow and maintain a separate adapter. Versioning schema? Incompatible API changes? Will have a careful look at the conversation https://github.com/bowsersenior/workflow_on_mongoid/pull/12 and think about improvement.

bowsersenior commented 10 years ago

Hi @geekq , thanks for the comments. I understand your position. The difficulty in adding an adapter to workflow is that I would have to monkeypatch the main workflow.rb file where the adapter is loaded. If there was a mechanism to register new adapters, then it would be much easier to package the mongoid adapter as a separate gem.

geekq commented 10 years ago

Hi Mani,

if we are talking about Mongo support in workflow, we are talking only about persistence of the workflow_state attribute and a before_validation hook, right?

klass.send :include, Adapter::Mongoid::InstanceMethods
klass.before_validation :write_initial_state

If so, there is no need for monkeypatching. We can squash this two additional lines into one and advise the user of workflow_on_mongoid library to use it as follows:

class MyDoc < Mongoid::Document
  include Workflow
  include Workflow::Mongoid

  workflow do
    ...
  end
end

In the workflow_on_mongoid the include-glue can be implemented roughly as:

module Workflow
  module Mongoid
    def self.included(klass)
      klass.send :include, Adapter::Mongoid::InstanceMethods
      klass.before_validation :write_initial_state
    end
  end
end

Can you try this out? If it does not work, I'll find time to fork workflow_on_mongoid to provide a working example of painless integration with workflow. ;-)

bowsersenior commented 10 years ago

Hi, I tried that out initially but there are problems and edge cases, such as when both Mongoid and ActiveRecord exist. The initial registering of the adapter needs to be improved to be more robust.

Anyways, it's clear you are hesitant to include this patch in workflow. Mongoid support does take a little extra support, but if you look at my pull request, I've already done the work for you.

I don't have the time to support workflow_on_mongoid any longer, so I'll just update the README to point people to alternative state machines like https://github.com/pluginaweek/state_machine which support Mongoid out of the box.

Thanks for your feedback.

geekq commented 10 years ago

Hi Mani,

if you do not have time/energy to support workflow-mongo, how should I take over the Mongo support? (integrate it into the core of workflow) I do not even use Mongo currently!

I'll just update the README

What you can also write, is if somebody wants to work on workflow_on_mongoid, I am ready to help him to integrate it with the workflow gem easily. Can debug together the issues, you mentioned ...