-
Couldn't load subscription status.
- Fork 4
Add support for dispatch_ids and queries filters
#213
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
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.
Pull Request Overview
This PR adds support for two new filtering parameters (dispatch_ids and filter_queries) to the dispatch client's list method, enabling more granular filtering of dispatches by specific IDs and text-based queries.
- Added
dispatch_idsparameter for filtering by specific dispatch IDs - Added
filter_queriesparameter for text-based filtering on dispatch ID and type fields with logical OR combining - Enhanced CLI with new options for the added filtering capabilities
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/frequenz/client/dispatch/_client.py | Added new filtering parameters and documentation to the list method |
| src/frequenz/client/dispatch/test/_service.py | Implemented filtering logic for dispatch_ids and queries in the fake service |
| src/frequenz/client/dispatch/main.py | Added CLI options for the new filtering parameters |
| tests/test_client.py | Added comprehensive test coverage for the filter_queries functionality |
| RELEASE_NOTES.md | Documented the new filtering features |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
74b90ad to
cb95cc6
Compare
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 didn't look into tests in much details. All minor comments, if you address them, feel free to force-merge after the push.
| return True | ||
|
|
||
| @staticmethod | ||
| def matches_query(_filter: DispatchFilter, dispatch: Dispatch) -> bool: |
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.
| def matches_query(_filter: DispatchFilter, dispatch: Dispatch) -> bool: | |
| def matches_query(filter_: DispatchFilter, dispatch: Dispatch) -> bool: |
Usually a leading _ is used for unused stuff, if you just want to use a keyword or reserved word in any way, a trailing _ is usually used. If in this case you to shut up pylint, I would add a pylint: disable= instead), otherwise the API looks ugly.
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 probably can make this a private method, too
| try: | ||
| query_id = DispatchId(int(query[1:])) | ||
| if dispatch.id == query_id: | ||
| matches_any_query = True | ||
| break | ||
| except ValueError: | ||
| # not an int, interpret as exact type match (without the #) | ||
| if query[1:] == dispatch.type: | ||
| matches_any_query = True | ||
| break |
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.
To make the error detection more fine-grained:
| try: | |
| query_id = DispatchId(int(query[1:])) | |
| if dispatch.id == query_id: | |
| matches_any_query = True | |
| break | |
| except ValueError: | |
| # not an int, interpret as exact type match (without the #) | |
| if query[1:] == dispatch.type: | |
| matches_any_query = True | |
| break | |
| try: | |
| int_id = int(query[1:]) | |
| except ValueError: | |
| # not an int, interpret as exact type match (without the #) | |
| if query[1:] == dispatch.type: | |
| matches_any_query = True | |
| break | |
| else: | |
| query_id = DispatchId(int_id) | |
| if dispatch.id == query_id: | |
| matches_any_query = True | |
| break |
| # Name of the parameter in client.list() | ||
| filters["target_components"] = target | ||
|
|
||
| # Convert dispatch_ids to iterator if present |
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.
It is pretty obvious you are converting to a iter, the question (what might be of more value in a comment) is why? :)
tests/test_client.py
Outdated
| assert dispatch == service_side_dispatch | ||
|
|
||
|
|
||
| # pylint: disable=too-many-locals |
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.
| # pylint: disable=too-many-locals | |
| # pylint: disable-next=too-many-locals |
Signed-off-by: Mathias L. Baumann <[email protected]>
| # Convert dispatch_ids to iterator to match client.list() parameter type | ||
| if "dispatch_ids" in filters: | ||
| filters["dispatch_ids"] = iter(filters["dispatch_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.
Again, super minor and irrelevant, but if you can call iter() on it, it is already an iterable, right? Why do you need to call it explicitly? It doesn't hurt to do so, so just out of curiosity...
Oh, wait, I just looked at client.list() and it takes an iterator instead of an iterable, I see. I think we could change it in the future to Iterable, any Iterator is iterable too, so it should work. It is pretty annoying not being able to pass a collection (iterable) directly to list() and having to explicitly convert to an iterator.
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 description provided.