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

Revert changes made to machine find_or_create method in #32 #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jonathan-wheeler
Copy link

PR #32 introduced functionality to always return a machine if one
is present and no key is supplied to the state_machine call.

This broke having multiple state machines on the same class unless
both have explicit names eg.using a slightly modified example from
the readme.

class Vehicle
  state_machine initial: :parked do
    event :crash do
      transition all => :parked
    end
  end

  state_machine :alarm_state, initial: :active, namespace: :'alarm' do
    event :enable do
      transition all => :active
    end
  end
end

The second machine would not be created and enable would be
declared on state, without the alarm namespace.

Tests were lacking to detect this behaviour so have added them.

An alternative fix would be to insist that classes with multiple
state machines always have a state key on each machine including
the default :state but I would argue that if you have a state
machine with a non default key then you would always supply the
key when interacting with the machine and have updated
test/functional/driver_default_nonstandard_test.rb
to reflect this.

Happy to make further amendments as you see fit. This is partially an
issue with a proposed fix. As I encountered a situation which was caught
out by this change and was unable to find other references to it. In the mean
time I have just added an explicit :state key on the default machine but am
unsure how is best to tackle it, so am open to discussion.

…s#32

PR state-machines#32 introduced functionality to always return a machine if one
is present and no key is supplied to the `state_machine` call.

This broke having multiple state machines on the same class unless
both have explicit names eg.using a slightly modified example from
the readme.

```
class Vehicle
  state_machine initial: :parked do
    event :crash do
      transition all => :parked
    end
  end

  state_machine :alarm_state, initial: :active, namespace: :'alarm' do
    event :enable do
      transition all => :active
    end
  end
end
```

The second machine would not be created and `enable` would be
declared on state, without the alarm namespace.

Tests were lacking to detect this behaviour so have added them.

An alternative fix would be to insist that classes with multiple
state machines always have a state key on each machine including
the default `:state` but I would argue that if you have a state
machine with a non default key then you would always supply the
key when interacting with the machine and have updated
test/functional/driver_default_nonstandard_test.rb
to reflect this.

Happy to make further amendments as you see fit.
@@ -4,7 +4,7 @@
class DriverNonstandardTest < MiniTest::Test
def setup
@driver = Driver.new
@events = Driver.state_machine.events
@events = Driver.state_machine(:status).events
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to pass the name now?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without it this call would create a new state machine on state.

Any call to state_machine when not using the default name (:state) needs to also supply the namespace. I could update to always return a state machine if there is one? But though it would be better to keep it simple and consistent.

Can also amend the read me to depending on which way you decide to go.

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