-
Notifications
You must be signed in to change notification settings - Fork 0
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
78: Add parameter relation config to simulator workflow in workflow_c… #83
base: main
Are you sure you want to change the base?
Conversation
…kflow-definition-including-smaller-and-greater-than
@@ -33,6 +33,13 @@ | |||
"parameter_type": "datetime", | |||
"key_name": "end_time" | |||
} | |||
], | |||
"parameter_relations": [ |
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.
Would parameter_constraints
be a suitable name? (I was thinking of how such a case would be handled in the world of SQL.)
Also wondering if instead of declaring parameter_relations
at this json level (which is completely fine), would it be more intuitive for users to define it at the parameter level. For instance:
{
"parameter_type": "datetime",
"key_name": "start_time",
"constraints: [
{
"relation": "smaller",
"key_2": "end_time"
}
]
"
},
Although not sure if it would make it more complicated in SDK code.
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.
Yes I agree, both with the 'constraints' naming and placing inside a parameter description. I would name key_2
as other_parameter
.
It would be more intuitive and you can control which of the two parameters is constraint, and therefore gets the warning. For instance, if end_time
is not larger than start_time
I only want a warning on the end_time
parameter for the mapeditor case. In some other situation you would want both parameters to get a warning.
@@ -33,6 +33,13 @@ | |||
"parameter_type": "datetime", | |||
"key_name": "end_time" | |||
} | |||
], | |||
"parameter_relations": [ |
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.
Yes I agree, both with the 'constraints' naming and placing inside a parameter description. I would name key_2
as other_parameter
.
It would be more intuitive and you can control which of the two parameters is constraint, and therefore gets the warning. For instance, if end_time
is not larger than start_time
I only want a warning on the end_time
parameter for the mapeditor case. In some other situation you would want both parameters to get a warning.
…onfig.json.
TODO: Still need to update SDK once released