-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
Introduce names for FiniteMachine instances #73
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for submitting this PR! Things are looking good. I added few comments. Most importantly, we need another test for when uuid
gets generated.
|
||
fsm.stop(1, 2) | ||
output.rewind | ||
expect(output.read).to match(/Transition: @event=stop @with=\[1,2\] yellow -> red/) | ||
expect(output.read).to match(/Transition: @machine=TrafficLights @event=stop @with=\[1,2\] yellow -> red/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metrics/LineLength: Line is too long. [110/80]
@@ -15,10 +15,10 @@ | |||
|
|||
fsm.slow | |||
output.rewind | |||
expect(output.read).to match(/Transition: @event=slow green -> yellow/) | |||
expect(output.read).to match(/Transition: @machine=TrafficLights @event=slow green -> yellow/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metrics/LineLength: Line is too long. [98/80]
|
||
expect(log).to have_received(:info).with("Transition: @event=go red -> green") | ||
expect(log).to have_received(:info).with("Transition: @machine=TrafficLights @event=go red -> green") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metrics/LineLength: Line is too long. [105/80]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on the suggestions. We still need another test for when the name isn't specified to ensure generating short UUID works correctly.
@@ -6,7 +6,7 @@ | |||
after { FiniteMachine.logger = ::Logger.new($stderr) } | |||
|
|||
it "logs transitions" do | |||
fsm = FiniteMachine.new log_transitions: true do | |||
fsm = FiniteMachine.new name: "TrafficLights", log_transitions: true do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a test case when the name is not specified. I'd probably keep this test as it was without the name and test the short UUID generation. Let me know if you need any pointers on how to test this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. I'll give it a shot and reach out if I run into trouble. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @piotrmurach: Just wanted to chime in and let you know that I got distracted but will get back to this in the next couple of days. Thanks.
Describe the change
This PR introduces the ability to specify an optional name for new FiniteMachine instances.
Why are we doing this?
Currently transition logs don't specify the FiniteMachine for which event transitions happen. This makes it hard to understand or post process the logs.
Benefits
It will now be possible to disambiguate event transitions that occur in the context of different FiniteMachine instances.
Drawbacks
None.
Requirements
master
branch?