-
Notifications
You must be signed in to change notification settings - Fork 41
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
[PY-407][PY-534][PY-404] Item + ItemQuery objects #715
Conversation
PY-407 Create ItemQuery object and relevant functions/filters
Create an ItemQuery object and functions and filters to work with items as needed.
Note: Should plan to only support Advanced Filters |
output += f"{k}={x}&" | ||
else: | ||
output += f"{k}={v}&" | ||
return output[:-1] # remove trailing & | ||
|
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 had to be changed and added to facilitate that a key can map to multiple values, eg dataset_id=0000&dataset_id=0001
@@ -75,7 +75,7 @@ def validate_fps(cls, values: dict) -> dict: | |||
elif isinstance(value, (int, float)): | |||
type = values.get("type") | |||
if type == "image": | |||
assert value == 0, "fps must be 0 for images" | |||
assert value == 0 or value == 1.0, "fps must be '0' or '1.0' for images" |
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.
turns out on our api, 1.0 also exists for items that are images. Found in dogfooding.
|
||
JSONType = Union[Dict[str, Any], List[Dict[str, Any]]] # type: ignore | ||
|
||
|
||
class Implements_str(Protocol): |
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 this protocol should mean it'll work with anything that can be converted to a string, so things like ints, floats etc will work.
|
||
def __init__(self, value: Dict[str, str]) -> None: | ||
def __init__(self, value: Mapping[str, list[Stringable] | Stringable]) -> 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.
Mapping here is a more free-form Dict. As per the mypy docs, it's better to use the constructs like Mapping/Sequence as input parameters, and more concrete ones like dict/list as return types.
... | ||
|
||
|
||
Stringable = Union[str, Implements_str] |
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'm not entirely sure why this union has to exist, but I couldn't get the type system to properly work without it. One would expect implements_str to encompass str natively, but it doesn't.
PY-534 Change stage.item_ids to stage.items
Currently, Change this to the actual stage objects, when possible. Acceptance Criteria
PY-404 Create delete method on item
Both items and query should have their own delete. (This applies to all batch actions) |
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.
Some comments for consideration, but overall LGTM
payload = { | ||
"filters": { | ||
"dataset_ids": dataset_ids | ||
"dataset_ids": [str(item) for item in dataset_ids] |
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.
Wait, so this accepts "all" as a possible input? That surprises me.
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.
all our endpoints do, although I have just removed that because I do think it's dangerous. Also it makes typing a bit of a nightmare anyway.
@@ -13,12 +11,23 @@ def test_querystring_happy_path() -> None: | |||
assert str(query_string_2) == "?foo=bar&baz=qux" | |||
|
|||
query_string_3 = QueryString({}) | |||
assert str(query_string_3) == "?" | |||
assert str(query_string_3) == "" |
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.
Yeah, protocols do work much better for this eh.
Problem
Item + ItemQuery doesn't exist, need to be added in so can manage and download these items. Also add in associated methods and features to objects that have Items as well as methods that items should have
Solution
Added in these objects
Changelog
Items now queryable with paginated support