Schedule tasks #2
Conversation
Reviewer's Guide by SourceryThis PR implements task scheduling functionality by adding new classes and endpoints to handle extract refresh tasks. The implementation includes a task hierarchy with a base TaskItem class and a specialized ExtractRefreshTaskItem class for handling extract refresh operations on datasources and workbooks. Class diagram for Tasks and ExtractRefreshes endpointsclassDiagram
class Endpoint {
}
class ExtractRefreshes {
- parent_srv
+ baseurl()
+ get_for_schedule(schedule_id, req_options)
}
class Tasks {
- parent_srv
- extract_refreshes
}
Endpoint <|-- ExtractRefreshes
Endpoint <|-- Tasks
Tasks o-- ExtractRefreshes
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @jacalata - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please provide a proper description of the changes and purpose of this PR. 'see what happens' is not sufficient documentation.
- Test coverage is missing for the new task scheduling functionality. Please add appropriate unit tests.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # a REST API. Possibly we could be more vigilant in hiding the constructor | ||
| def __init__(self, id, schedule_id, priority, refresh_type, item_type, item_id): | ||
| super(ExtractRefreshTaskItem, self).__init__(id, schedule_id) | ||
| self.priority = priority |
There was a problem hiding this comment.
issue (bug_risk): The 'type' parameter in init is assigned to self.type but never used elsewhere in the class
This unused assignment could lead to confusion and potential bugs. Consider removing it if not needed.
| @staticmethod | ||
| def _parse_element(extract_tag): | ||
| id = extract_tag.get('id') | ||
| priority = int(extract_tag.get('priority')) |
There was a problem hiding this comment.
issue: Missing error handling for malformed XML attributes in _parse_element
Add error handling for cases where priority is not a valid integer or other attributes are missing/malformed.
| @item_type.setter | ||
| def item_type(self, value): | ||
| # Check that it is Datasource or Workbook | ||
| if not (value in [ItemTypes.Datasource, ItemTypes.Workbook]): |
There was a problem hiding this comment.
suggestion (code-quality): Simplify logical expression using De Morgan identities (de-morgan)
| if not (value in [ItemTypes.Datasource, ItemTypes.Workbook]): | |
| if value not in [ItemTypes.Datasource, ItemTypes.Workbook]: |
|
|
||
| @classmethod | ||
| def from_response(cls, resp, schedule_id): | ||
| tasks_items = list() |
There was a problem hiding this comment.
suggestion (code-quality): Replace list() with [] (list-literal)
| tasks_items = list() | |
| tasks_items = [] |
Explanation
The most concise and Pythonic way to create a list is to use the[] notation.
This fits in with the way we create lists with elements, saving a bit of mental
energy that might be taken up with thinking about two different ways of creating
lists.
x = ["first", "second"]Doing things this way has the added advantage of being a nice little performance
improvement.
Here are the timings before and after the change:
$ python3 -m timeit "x = list()"
5000000 loops, best of 5: 63.3 nsec per loop
$ python3 -m timeit "x = []"
20000000 loops, best of 5: 15.8 nsec per loop
Similar reasoning and performance results hold for replacing dict() with {}.
|
|
||
| @staticmethod | ||
| def _parse_element(extract_tag): | ||
| id = extract_tag.get('id') |
There was a problem hiding this comment.
issue (code-quality): Don't assign to builtin variable id (avoid-builtin-shadow)
Explanation
Python has a number ofbuiltin variables: functions and constants thatform a part of the language, such as
list, getattr, and type(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:
list = [1, 2, 3]However, this is considered poor practice.
- It will confuse other developers.
- It will confuse syntax highlighters and linters.
- It means you can no longer use that builtin for its original purpose.
How can you solve this?
Rename the variable something more specific, such as integers.
In a pinch, my_list and similar names are colloquially-recognized
placeholders.
see what happens
Summary by Sourcery
Add support for scheduling and managing extract refresh tasks by introducing new classes and endpoints. Enhance error handling with a new exception class for response body errors.
New Features:
ExtractRefreshTaskItemto represent extract refresh tasks, including properties for priority, refresh type, item type, and item ID.Tasksclass to the server module, which includes anExtractRefreshesendpoint for handling extract refresh tasks.Enhancements:
ResponseBodyErrorto handle errors related to response bodies.