Implement feedback filtering in CLI & SDK #206
Conversation
nullfox
commented
Jul 1, 2024
•
nullfox
commented
log10/feedback/feedback.py
Outdated
|
|
||
| try: | ||
| while True: | ||
| fetch_limit = limit if limit else 50 |
There was a problem hiding this comment.
This can be written as a default arg eg
def _get_feedback_list_graphql(page, task_id, filter, limit=50):
log10/feedback/feedback.py
Outdated
|
|
||
| def _get_feedback_list_graphql(page, limit, task_id, filter): | ||
| feedback_data = [] | ||
| current_page = page if page else 1 |
There was a problem hiding this comment.
Can set page=1 as a default arg (see below)
log10/feedback/feedback.py
Outdated
|
|
||
| variables = { | ||
| "id": self._log10_config.org_id, | ||
| "taskId": task_id or None, |
There was a problem hiding this comment.
The default value for task_id is empty string, is there significance to None or does it just need to be falsey? Another approach is to default it to None. (Just something that jumped out from a readability standpoint)
log10/cli/feedback.py
Outdated
| "--filter", | ||
| default="", | ||
| type=str, | ||
| help="The filter applied to the feedback. If not provided, feedback will not be filtered.", |
There was a problem hiding this comment.
can we add a bit explanation here, even a simple sentence like "e.g. --filter "coverage > 4"".
There was a problem hiding this comment.
do you think that an explanation is still needed here?
log10/feedback/feedback.py
Outdated
| logger.error(e.response.json()["error"]) | ||
| return [] | ||
|
|
||
| return list(map(_format_graphql_node, feedback_data)) |
There was a problem hiding this comment.
| return list(map(_format_graphql_node, feedback_data)) | |
| return [_format_graphql_node(item) for item in feedback_data] |
more readable in python sense. Also I realized that we have used list as a function in Feedback class. We should do a follow up to rename that function, so that we don't name function the same as python build-in function.
log10/feedback/feedback.py
Outdated
| feedback_data = [] | ||
| current_page = page if page else 1 | ||
| if limit: | ||
| limit = int(limit) |
There was a problem hiding this comment.
nit: thinking to add the default 50 value here.
else:
limit = 50
so that line 183 could be fetch_limit = limit in line 183. or just directly use limit in the while loop
log10/feedback/feedback.py
Outdated
| current_fetched = len(new_data) | ||
| current_page += 1 | ||
|
|
||
| if current_fetched < fetch_limit: |
There was a problem hiding this comment.
just wonder if there's edge case, like when current_fetched = 50 and fetch_limit = 50, so it would go to next iteration?
log10/feedback/feedback.py
Outdated
| } | ||
|
|
||
|
|
||
| def _get_feedback_list_graphql(page, task_id, filter, limit=50): |
There was a problem hiding this comment.
could you set page=1 as a default?
There was a problem hiding this comment.
done.
btw, what's the API for task_id and filter. should I set the default to None or ""?
log10/cli/feedback.py
Outdated
| "--filter", | ||
| default="", | ||
| type=str, | ||
| help="The filter applied to the feedback. If not provided, feedback will not be filtered.", |
There was a problem hiding this comment.
do you think that an explanation is still needed here?
