Normalize functions to all take arguments #238
Conversation
With all functions able to take parameters, then we can do argument checking in the transformer. This commit adds argument checking for States.UUID.
Fryguy
force-pushed
the
normalize_function_calls
branch
from
47c24cb to
21f1dac
Compare
|
Checked commit Fryguy@21f1dac with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
|
I think passing arguments to a function that doesn't take one is a payload problem not a runtime problem (like an argument being a string instead of an int or something) so InvalidWorkfloeError would make sense |
|
@agrare Maybe? If it's hardcoded I would agree, but if it's inferred, it's a runtime problem I think. Compare |
|
I've found it is hard to guess when aws will throw a runtime exception and when they will not. In some cases, doing a StringEquals doesn't throw an exception when a StringGreaterThan will. (where the arguments are a string and an integer) -- you can see a few examples in that fixtures file I had. I think using guard clauses (via Choice) and checking types is the only way around these nuances. |
|
I wasnt trying to follow aws here, but maybe that's a good idea to see what the simulator does |
|
@agrare if it's ok, can we merge for now with the regular ArgumentError? Reason is I'm starting to do others and patterns are emerging. I think it's clearer what choices to make with more examples. I'm also seeing that I can normalize most of the arity and type checks. |
I was thinking more along the lines of if someone wrote |
|
|
||
| rule(:states_uuid => {:args => subtree(:args)}) do | ||
| args = Transformer.resolve_args(args()) | ||
| raise ArgumentError, "wrong number of arguments to States.UUID (given #{args.size}, expected 0)" unless args.empty? |
There was a problem hiding this comment.
I do think this should be Floe::InvalidWorkflowError instead of ArgumentError, or maybe we catch ArgumentError while evaluating the intrinsic_function and raise InvalidWorkflowError then?
There was a problem hiding this comment.
Yeah I'm seeing patterns where I check arity (which I agree is a workflow error), then types (which feels more like a runtime error), and then specific conditions like argument must be non-zero positive (which also feels like a runtime error)
I agree 100% that all of those errors we should wrap, and will do in a follow up
There was a problem hiding this comment.
I check arity (which I agree is a workflow error), then types (which feels more like a runtime error), and then specific conditions like argument must be non-zero positive (which also feels like a runtime error)
Completely agree on these
Added - Set Floe.logger.level if DEBUG env var set (#234) - Add InstrinsicFunction foundation + States.Array, States.UUID (#194) - Evaluate IntrinsicFunctions from PayloadTemplate (#236) - Add more intrinsic functions (#242) - Add State#cause_path, error_path (#249) - Validate Catcher Next (#250) - Add spec/supports (#248) Fixed - Handle non-hash input/output values (#214) - Fix Input/Output handling for Pass, Choice, Succeed states (#225) - Wrap Parslet::ParseFailed errors as Floe::InvalidWorkflowError (#235) - Fix edge cases with States.Array (#237) - Fix sporadic test failure with Wait state (#243) - Fix invalid container names after normalizing (#252) Changed - Extract ErrorMatcherMixin from Catch and Retry (#186) - Output should be JSON (#230) - Normalize functions to all take arguments (#238) - Pass full path name to State.new for better errors (#229) - Validate that state machine input is valid JSON (#227) - Move the Parslet parse into initialize so that invalid function definition will fail on workflow load (#245)
With all functions able to take parameters, then we can do argument checking in the transformer. This commit adds argument checking for States.UUID.
@agrare Please review.
This allows for the separation of syntax errors from semantic errors.
I did this PR separate as a precursor to States.ArrayPartition where I actually have to check arity and specific argument types. States.UUID shows how that can be achieved.
Not sure if we want to raise ArgumentError or something else? Thoughts?