-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Implement Pydantic model for workflow test format. #18884
Conversation
General purpose tooling will likely require more union models in that syntax and narrowing will take more effort with less readable results. That's the reason I don't love it. They should however all validate, I agree 100% there. |
Ah, my comment was about the |
|
||
that: Literal["has_n_lines"] = "has_n_lines" | ||
class base_has_n_lines_model(AssertionModel): | ||
"""base model for has_n_lines describing attributes.""" |
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.
The result of all of them being optional is that null or no value or nonsense like only delta
or only negate
validates.
It's not a huge problem, and the fix isn't super clean. To model this fully base_has_n_lines_model
would have to be a non-optional union of options that make sense, so something like (n + delta? + min? + max? + negate?) | (min + n? + delta? + max? + negate?) | (max + n + delta? + min? + negate?)
.
I suppose the current base_has_n_lines_model
could be the base class that isn't exposed, and then you subclass to create the individual permissible union members with required properties. If you think that's worth doing I'm happy to try.
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.
This is a very good point. I think I could solve this with Pydantic validators and some Annotation/decorator syntax for describing mutually exclusive but required arguments on the assertions functions but that wouldn't come through in your JSON schema land I suppose... so I see you preference for the unions. Hmm.... feel free to give it a shot or if you want to outline how you think it should be annotated on the parameters, I could try to work through the details also.
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.
Also bonus points if you can generate those same unions in the XSD huh? It almost certainly has the same issues.
If anyone wants to explore the schema when editing test files you can install the redhat yaml extension in vscode and put this in your settings.json file:
This is amazing! |
All the other assertions read like English to me - has n lines where |
This adapts the assertion models to allow any of the three YAML formats:
I don't love this - but they're all allowed currently and all work. Likewise the models allow either
children
orasserts
for child assertion keys and eitherelements
orelement_tests
for the element test definitions. I think it might be a good idea to have a cleaned up version that only allows one kind of assertion, one kind of child assertion, and one key for elements. I have my preferences there but I think that is subsequent PR and one that requires more politicking. This PR isn't about "best practices" - it is about "what currently works".I also "un-deprecated"
I much prefer:
but the IWC had a ton of instances of the prior syntax.
As pointed out in https://github.com/galaxyproject/iwc/pull/530/files/07d11e07e9f470fbd05e999c9773d0b473b52a56#diff-bae1f71ab2c19af3da6c4a361095035ff4a7838279decd67161917f5234bfddc - I think a follow up PR should implement a cleaner syntax for specifying assertions about collections also.
How to test the changes?
(Select all options that apply)
License