feat: Conditional parameters (HEXA-1687)#396
Conversation
DimitriKwihangana
left a comment
DimitriKwihangana
left a comment
There was a problem hiding this comment.
It looks nice! @mrivar only minor comments I've put!
| "required": True, | ||
| "directory": None, | ||
| "disables": None, | ||
| "disableWhen": True, |
There was a problem hiding this comment.
Minor! I was thinking if we matched both the attribute and this serialisation key to snake_case. for the consistency. I don't where I got this but I see mostly people use Camelcase in Javascript 😂. no strong opinion
There was a problem hiding this comment.
oh yeah, this is quite weird being code in python 😂 this is actually because the .dict() is used as values to be sent to our graphQL endpoint, so it is expecting camelCase. Using snake_case (which would be the normal thing in python) breaks it.
I learned this the hard way, broke it in the other task I did about the dynamic params and @bramj fixed it here: #393
| }, | ||
| ) | ||
|
|
||
| def test_pipeline_with_disables_param(self): |
There was a problem hiding this comment.
Not sure if we have it but I was expecting a test for the scenario when a required parameter is disabled
There was a problem hiding this comment.
I'll check again and make sure! I think we do but it's always good to double check 😁
Edit: I'm checking and I think we have them in test_pipeline.py, check test_pipeline_run_disabled_required_parameter_skipped and test_pipeline_run_disable_when_false_disables_while_off (for the inverted version), but if you find any other coverage that might be missing lmk!
2 participants
Related PRs
Add posibility to have a param that disables others.
Changes
disables: List of parameter to disable.disable_when: Setting to disable the linked params when the parameter is either true or false. True by default.