Skip generating methods that State Machine does not generate

[bitwise-aiden]

Motivation

When using the state machine gem it is encouraged to use either String or Symbol for state types, but this isn't strictly enforced.

class Vehicle
  ACTIVE = 1
  OFF = 0

  state_machine :alarm_state, initial: ACTIVE do
    before_transition any => ACTIVE do |unit|
      unit.some_cool_method_at = Time.now.utc
    end

    event :enable do
      transition all => ACTIVE
    end

    event :disable do
      transition all => OFF
    end

    state ACTIVE
    state OFF
  end
end

If someone were to use a different type like in this example, the type generated for alarm_state= would still be T.any(String, Symbol) despite actually wanting Integer. There are a few other methods that suffer this as well.

This stems from using State#value to determine the states to use. This value is set as a to_s version of the name unless otherwise specified (source). What this means practically is that if in the example above someone were to write vehicle.alarm_state = Vehicle::OFF they would get an error.

It appears as though everything is triple indexed, once by raw value of name then by the string and symbol versions too (source). So this is how it still allows it to work despite having value set to the stringified version.

Implementation

I've updated the #state_type_for method to use name instead of value to get the correct type. This is then passed into wherever it is needed to properly validate.

I'd originally check if the value is a string version of name and if so returning name instead, but this doesn't work for cases where value is provided to the state, like so:

state_machine do
  state :active, :value => 1
end

This would then fail because value would be the Integer value of 1 causing Integer to be the type for state= which won't work for state = :inactive.

Question for reviewers

I've been pondering if the state methods for the machine should also include String and Symbol by default because they can have those. I'm leaning toward the current implementation because if all states are defined with a single type then it shouldn't expect any others. Are there any thoughts on this?

Tests

Test added for new case.

[paracycle]

With your example of:

class Vehicle
  ACTIVE = 1
  OFF = 0

  state_machine :alarm_state, initial: ACTIVE do
    before_transition any => ACTIVE do |unit|
      unit.some_cool_method_at = Time.now.utc
    end

    event :enable do
      transition all => ACTIVE
    end

    event :disable do
      transition all => OFF
    end

    state ACTIVE
    state OFF
  end
end

you get:

tapioca> v = Vehicle.new
#=> #<Vehicle:0x000000012a47f1a0 @alarm_state="1">

tapioca> v.alarm_state
#=> "1"

tapioca> v.alarm_state = 1
#=> 1

tapioca> v.alarm_state
#=> 1

tapioca> v.alarm_state = "1"
#=> "1"

tapioca> v.alarm_state
#=> "1"

tapioca> v.alarm_state = Object.new
#=> #<Object:0x000000012a41e3c8>

tapioca> v.alarm_state
#=> #<Object:0x000000012a41e3c8>

tapioca> v.disable
#=> true

tapioca> v
#=> #<Vehicle:0x000000012a47f1a0 @alarm_state="0">

tapioca> v.alarm_state
#=> "0"

So this is much more complicated than it seems. However, it seems internally the state machine uses string values when doing state changes, so I guess we should be forcing folks to use string values as well.

[bitwise-aiden]

So this is much more complicated than it seems. However, it seems internally the state machine uses string values when doing state changes, so I guess we should be forcing folks to use string values as well.

Ahh, great point! What about having parameters be type + String + Symbol then return values being just String? That way people can still index into it in their expected way without having to change code.

I was originally going to say keeping it as is to preserve any issues that may arise, but I think this is actually the better approach driving folks that want a more exact type to define using state SOME_CONSTANT, value: 1.

In that case I'll update the compiler to use string for return keeping symbol/string for input.

[paracycle]

I was originally going to say keeping it as is to preserve any issues that may arise, but I think this is actually the better approach driving folks that want a more exact type to define using state SOME_CONSTANT, value: 1.

In that case I'll update the compiler to use string for return keeping symbol/string for input.

Yes, that sounds good to me.

[bitwise-aiden]

Hmm, using the value system means that the return of the state method will be that type. So we are going to have a similar issue in that the setter is String and getter is whatever the value of that type is.

Also seems like there may be different behaviour on ActiveRecord models. I was looking through the repo for the integration, but nothing jumps out at me: https://github.com/state-machines/state_machines-activerecord/tree/master

This part of the docs talks about how you're able to override the value to not be a string, trying to see if I can find that used anywhere but also think about how that interacts with this.

[bitwise-aiden]

When using with AR I think what is happening is that the attribute writer isn't being created in the case that the table has a field by the same name as the machine (source).

I think in the case of integers this will then convert the string to an integer meaning it will return the "correct" type.

@paracycle What do you think we should do for this then?

[bitwise-aiden]

I've updated the implementation to omit the state accessor generated for the machine if they have already been defined by the owning class.