-
Notifications
You must be signed in to change notification settings - Fork 6
DSL sample #37
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
DSL sample #37
Conversation
| if input.is_a?(String) | ||
| # Run YAML parsing in an unsafe block since it's non-deterministic | ||
| Temporalio::Workflow::Unsafe.illegal_call_tracing_disabled do | ||
| require 'yaml' |
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.
I think we should add this dependency in the Gemfile, and say it's part of group :dsl. I think the require should be at the top of the file (though as mentioned in another comment, workflow shouldn't have to know anything about YAML IMO in this sample, but can be discussed).
| # Safely parse the input | ||
| def parse_input(input) | ||
| if input.is_a?(String) | ||
| # Run YAML parsing in an unsafe block since it's non-deterministic |
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.
I think YAML parsing needs to be done by caller and the workflow should accept a serializable model instead of a YAML string. The workflow shouldn't concern itself with the file format IMO. We should have the client do the parsing. I do understand this makes it easier for CLI, but I wonder if that should be an exercise left to the reader.
| # frozen_string_literal: true | ||
|
|
||
| module Dsl | ||
| module Models |
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.
Module name and file name should match
| end | ||
|
|
||
| # Parse YAML to DSL models | ||
| class Parser |
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.
Where is this class used?
| futures.map(&:result) | ||
| end | ||
|
|
||
| def activity_name_to_class(name) |
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.
I'd just capitalize the names in the YAML and remove this conversion. No need for this extra code/complication in the sample IMO.
|
Went ahead and copied/rewrote this in #52 |
What was changed
Added a DSL sample.
Checklist
Closes Sample request: DSL workflow #32
How was this tested:
Run manually using README.