feat: case name templating #795
Conversation
ChibiBlasphem
force-pushed
the
christopher/case-name-templating
branch
2 times, most recently
from
fa6c2c6 to
6ec80f2
Compare
There was a problem hiding this comment.
I think the implementation seems about right to me, as far as I can judge from reading.
I left a few comments, but rather minor. The one thing I'm uneasy with is the refactor that touches "isAuthorizedError".
Can we clarify that the template ast is supposed to stay nullable, and we keep the default case name template hard coded ?
| astEvaluation, _ := ast_eval.EvaluateAst(ctx, dryRunEnvironment, *astNode) | ||
| astEvaluationReturnType := reflect.TypeOf(astEvaluation.ReturnValue) | ||
|
|
||
| if len(astEvaluation.FlattenErrors()) == 0 && astEvaluationReturnType != returnType { |
There was a problem hiding this comment.
Here, I rather think that you wanted to first check on len(astEvaluation.FlattenErrors()) > 0 and then on astEvaluationReturnType != returnType?
There was a problem hiding this comment.
I wanted to append return type error only if there's no other errors in the evaluation. Something I noticed is that when I get an evaluation error I always get the return type wrong.
There was a problem hiding this comment.
From how you use the function, I still think that there is a possible case where you get a non-empty astEvaluation.FlattenErrors() but it is passed silently.
That being said, the whole structure of the astEvaluation and functions that use it may benefit from being reworked a bit, especially how we pass errors. But that would be left for another PR and probably rather for me.
ChibiBlasphem
force-pushed
the
christopher/case-name-templating
branch
from
6ec80f2 to
da3af7f
Compare
usecases/scenario_usecase.go
Outdated
| @@ -82,6 +87,13 @@ func (usecase *ScenarioUsecase) UpdateScenario( | |||
| return models.Scenario{}, err | |||
| } | |||
|
|
|||
| if scenarioInput.DecisionToCaseNameTemplate != nil { | |||
| if validation, _ := usecase.ValidateScenarioAst(ctx, scenarioInput.Id, | |||
There was a problem hiding this comment.
ValidateScenarioAst can also return a non-nil error, this case should also be handled
decision_to_case_name_templatein scenario table/scenario/:scenarioId/ast-validationto validate an AST nodedecision_to_case_name_templateFrontend PR: checkmarble/marble-frontend#647