gocardless / statesman

A statesmanlike state machine library.
https://gocardless.com/blog/statesman/
MIT License
1.78k stars 163 forks source link

Adds ActiveSupport::StringInquirer-like behaviour for states. #304

Closed Greyschale closed 5 years ago

Greyschale commented 6 years ago

Sometimes it's more convenient to have inquirer methods for known states, rather than defining methods manually for all of them using #in_state?.

For example, a model with states [:pending, :active, :inactive] may need to have defined:

def pending?
  in_state?("pending")
end

def active?
  in_state?("active")
end

def inactive?
  in_state?("inactive")
end

With this changes, these methods won't need to be explicitly defined on the model.

This uses similar #method_missing and #respond_to_missing? overrides as the official ActiveSupport::StringInquirer, but also makes sure to check the model's existing states as well.

danwakefield commented 6 years ago

I don't mind this in theory but this PR would make it a breaking change. Is there a way we can disable it by default and put it behind a feature flag?

dmagliola commented 6 years ago

I'm not too thrilled about magically adding methods to people's classes. This may be a bit non-obvious. On the other hand, it's method_missing, so if the class defines one of these already, we're not interfering with that. So it shouldn't cause problems, I believe, but I feel a bit icky about it anyway.

I'd add some sort of DSL-y method call to activate this for the models in which you want it.

class OrderStateMachine
  include Statesman::Machine

  enable_magic_inquirer_methods

  state :pending, initial: true
  state :checking_out
end

Just my 2p, though, I'm perfectly ready to admit i'm wrong / this is a bad idea...

barisbalic commented 6 years ago

I've defined methods like these in the past myself, I've not yet felt like it needed to be added to statesman directly. It's not a particularly scary change, but it is adding more 'magic' for syntactic sugar, and it's not something users have asked for previously which leads me to believe it's maybe not worth pulling into statesman, but I'm happy to be convinced otherwise.

@dmagliola I hear what you are saying, but I wouldn't like the arbitrary invocation "activating" methods any more than the default magic.

Another option to pushing this into statesman would be to create a separate gem that acts as an extension. But again, would be happy for other contributors to say otherwise.

danwakefield commented 5 years ago

Going to close as its been ~2 years. Copying my comments from an internal PR.

So there where some recent changes to statesman to make the ActiveRecordQueries mixin better. I think we could add another key to the config. e.g

include Statesman::Adapters::ActiveRecordQueries[
  transition_class: OrderTransition,
  initial_state: :initial,
  helpers: true
]

which would define all class scopes & instance methods based on the state machine.

include Statesman::Adapters::ActiveRecordQueries[
 transition_class: OrderTransition,
 initial_state: :initial,
 helpers: [:foo, :bar]
]

Would only define the Order.foo / Order.bar scopes and o.foo? / o.bar? methods.

This merges xxx's suggestion of being configurable with an easy way to default to defining all methods without lots of copy pasting of state names. Its still magic, but its magic you have to choose to enable.