-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add query runs method #107
Conversation
It'd be nice to change |
Did you consider using a library for the polymorphic filter type? E.g. https://github.com/marshmallow-code/marshmallow-oneofschema or https://github.com/Bachmann1234/marshmallow-polyfield. (I'm not saying we need to use it). |
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.
Well done, it guess it must be quite fiddly to write custom marshmallow schemas... I left a few comments. Also I'm not sure about marshmallow best practices around polymorphic types, so someone else's opinion might be useful.
@tomas-milata I agree that we should do it, but I was considering doing it in a separate PR. My reasoning is that the In fact I would say that, other than |
@tomas-milata I did not, but I after taking a look I think that:
|
Actually I think my points on the first one are wrong. That one might be a viable solution |
94d830d
to
9aa55ac
Compare
bf5f8ab
to
7a83f8c
Compare
* Use marshmallow's `field.fail()` mechanism * Map fitler types to field types with dict * Factor out param filter type polymorphism
faculty/clients/experiment.py
Outdated
|
||
class CompoundFilterSchema(BaseSchema): | ||
operator = EnumField(CompoundFilterOperator, by_value=True, required=True) | ||
conditions = fields.List(FilterField(skip_if=None)) |
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.
What does skip_if
do? I can't find any mention of it in the documentation.
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.
I think this was added by mistake as all tests passed when I tried removing it. Thanks for spotting this out, I will remove it now.
…ntation to context in which it is used
These methods were unused, untested, and in some cases broken.
Refactor filter and sort objects and associated schemas
No description provided.