-
Notifications
You must be signed in to change notification settings - Fork 8
Schema Validation and UX improvements #4
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
Conversation
| }, | ||
| "refresh": { | ||
| "type": "string" | ||
| } |
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.
Are these the auth JWT's?
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 👍
| } | ||
| } | ||
| } | ||
| } |
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.
Whew what an adventure~
| schema = None | ||
|
|
||
| @classmethod | ||
| def validate(cls, candidate): |
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.
So even when derived classes call BaseResponse's implementation of validate, cls.schema would be the child's schema right?
ie: VersionResponse would have "version.json" as it's class schema.
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.
Exactly. schema is a class property, meaning that it's the same (pointer to) the data for each class. validate then just hooks into that data and interprets it as a schema.
Because the base classes are abstract, we cannot instantiate them directly, so as long as our concrete class definitions override schema with the stuff they need, we should be fine.
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.
However, there's a hidden bug here. Classes which don't override schema and don't implement validate will fail, because their schema variable is None and the default validate implementation is executed. So in the default implementation we should add another check for None and raise a validation error to notify the user about a missing schema. WDYT?
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.
Good catch
pythx/models/request/base.py
Outdated
| @classmethod | ||
| def validate(cls, candidate): | ||
| if cls.schema is None: | ||
| raise TypeError("Cannot use BaseRequest.validate without a schema") |
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.
Good informative error but I think we could abstract away the extra info. This error will mostly like occur when a derived class tries to call cls.validate(d) without setting the schema.
I'm unsure if we should give reveal too much of our implementation details.
Rephrasing to reveal the most relevant info:
if cls.schema is None:
raise TypeError("Cannot validate request without a schema. Please load schema")
What do you think?
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.
Good point. This error will occur when a domain model doesn't have a JSON body (e.g. logout responses or analysis status requests). Maybe we should also bring this in somehow? Otherwise the user will be confused if we ask them to load a schema even though there can't be one. It's most likely a mistaken validate call
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 it be possible to overridevalidate for those models then?
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. That's always possible in Python, after all we are all consenting adults here. 👍
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.
Ohmy that's pretty different from what's taught in class 😂 (The encapsulation part I mean)
This PR will add:
Furthermore, the
*DecodeErrorexceptions have been deprecated in favour of the*ValidationErrorones. This makes it easier to catch internal PythX errors. As discussed before, an example like this is now possible:Next up: Further UX improvements, examples in the readme, actual documentation, and then v1.0 🎉