-
-
Notifications
You must be signed in to change notification settings - Fork 423
Expand the supported data types for filter values in Mast.mast_query
#3422
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3422 +/- ##
==========================================
+ Coverage 70.54% 70.57% +0.02%
==========================================
Files 232 232
Lines 20022 20037 +15
==========================================
+ Hits 14125 14141 +16
+ Misses 5897 5896 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ba29e41
to
81160bf
Compare
Mast.mast_query
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.
Overall looks good, I have only one questions about the server being able to handle if there are more than min/max keys in the input dictionary.
Also needs a rebase to resolve the changelog conflict
---------- | ||
key : str | ||
Parameter name (used for error messages). | ||
val : any |
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.
nitpick: general guideline to always spell out names, we don't need to save characters
warnings.warn("'columns' parameter will not mask non-filtered services", InputWarning) | ||
warnings.warn( | ||
"'columns' parameter is ignored for non-filtered services.", | ||
InputWarning | ||
) |
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.
no need to change it back but in general we allow longer lines (120), so no need to cut up perfectly valid one liners
""" | ||
# Range filters must be dicts with 'min' and 'max' | ||
if isinstance(val, dict): | ||
if not {"min", "max"}.issubset(val.keys()): |
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 happens when value has more than min/max, would the server handle dropping/using the extra ones?
Previously, for filters not related to position or request in
Mast.mast_query
, users had to enclose filter values in a list for the expected response from the server. This PR handles the following cases so that filters are passed in correctly:I also added more documentation, since this was unclear to users before.