ManageIQ / floe

Floe is a runner for Amazon States Language workflows
Apache License 2.0
0 stars 5 forks source link

Normalize functions to all take arguments #238

Closed Fryguy closed 2 months ago

Fryguy commented 2 months ago

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?

miq-bot commented 2 months ago

Checked commit https://github.com/Fryguy/floe/commit/21f1dacf952b155d4b6f07ed26489627021c91ec with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 4 files checked, 0 offenses detected Everything looks fine. :cake:

agrare commented 2 months ago

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

Fryguy commented 2 months ago

@agrare Maybe? If it's hardcoded I would agree, but if it's inferred, it's a runtime problem I think.

Compare States.ArrayContains(States.Array(1, 2, 3), -1) vs States.ArrayContains($.input, $.target) with an input of {:input => [1, 2, 3], :target => -1}. The latter is a runtime error I would think not a workflow error.

kbrock commented 2 months ago

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.

Fryguy commented 2 months ago

I wasnt trying to follow aws here, but maybe that's a good idea to see what the simulator does

Fryguy commented 2 months ago

@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.

agrare commented 2 months ago

Compare States.ArrayContains(States.Array(1, 2, 3), -1) vs States.ArrayContains($.input, $.target) with an input of {:input => [1, 2, 3], :target => -1}. The latter is a runtime error I would think not a workflow error.

I was thinking more along the lines of if someone wrote States.UUID(1) that for sure is a definition error not a runtime error