Xin.L-Task_list_api #29
Conversation
app/models/goal.py
Outdated
| def to_dict(self): | ||
| return { | ||
| "id": self.id, | ||
| "title": self.title | ||
| } | ||
|
|
||
| def to_dict_with_tasks(self): | ||
| return { | ||
| "id": self.id, | ||
| "title": self.title, | ||
| "tasks": [task.to_dict_with_goal_id() for task in self.tasks] | ||
| } |
There was a problem hiding this comment.
The function to_dict_with_tasks is duplicating effort from to_dict around creating a dict and setting up the id and title keys.
How can we D.R.Y. up to_dict_with_tasks by calling from_dict first to avoid repeating the tasks it already handles?
app/models/task.py
Outdated
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] | ||
| description: Mapped[str] | ||
| completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True) |
There was a problem hiding this comment.
nullable=False is the default value for an attribute in SQLAlchemy. It is more common to leave off the mapped_column content unless we are changing a setting to something other than the default value.
app/models/task.py
Outdated
| def to_dict(self): | ||
| return { | ||
| "id": self.id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": self.completed_at is not None | ||
| } | ||
|
|
||
| def to_dict_with_goal_id(self): | ||
| return { | ||
| "id": self.id, | ||
| "goal_id": self.goal_id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": self.completed_at is not None | ||
| } |
There was a problem hiding this comment.
The feedback in Goal.py around multiple to_dict functions applies here as well.
How can we refactor these functions to reduce repetition?
- Do we need 2 functions, or could we create the dictionary with the required attributes and only add the
goal_idif one exists before we return the dictionary?
| return cls( | ||
| title=data["title"], | ||
| description=data["description"], | ||
| completed_at=data.get("completed_at") |
There was a problem hiding this comment.
Great use of get to access the optional attribute.
app/routes/goal_routes.py
Outdated
|
|
||
|
|
||
| @bp.get("") | ||
| # need to refactor later |
There was a problem hiding this comment.
Notes to ourselves throughout code should be cleaned up before opening PRs.
If this comment is still relevant, what were you looking to refactor?
| @@ -0,0 +1,32 @@ | |||
| """empty message | |||
There was a problem hiding this comment.
I am seeing a lot of empty messages in the migration files. Just like with our git commits we should be leaving a descriptive message of changes every time we migrate to help other developers easily be able to track the changes to the database structure over time.
app/routes/task_routes.py
Outdated
| request_body = request.get_json() | ||
|
|
||
| if not request_body: | ||
| return {"details": "Invalid data"}, 400 |
There was a problem hiding this comment.
The feedback in Goal.py around using abort to send error responses applies here as well. How would we need to update this function to be consistent across the project and follow best practices for sending error responses?
app/routes/task_routes.py
Outdated
| response = requests.post( | ||
| url="https://slack.com/api/chat.postMessage", | ||
| headers={"Authorization": f"Bearer {token}"}, | ||
| json={"channel": "task-notifications", | ||
| "text": f"Someone just completed the task {task.title}"}) |
There was a problem hiding this comment.
It doesn't look like we are reading anything from response after it is created, so I would strongly consider not creating a variable to hold the Slack message response in this situation.
- In a larger project we would likely inspect that value in some way, so then we would want to store it to work with it later.
| def create_model(cls, model_data): | ||
| try: | ||
| new_model = cls.from_dict(model_data) | ||
| except KeyError as error: | ||
| response = {"details": "Invalid data"} | ||
| abort(make_response(response, 400)) | ||
|
|
||
| db.session.add(new_model) | ||
| db.session.commit() | ||
|
|
||
| return new_model.to_dict(), 201 |
There was a problem hiding this comment.
Since this both creates a model and creates an http response, I'd consider renaming this to create_model_and_response. Or to better follow single responsibility principles, we could split this into 2 functions:
- one function that only manages creating and returns the new model
- one function that uses the function above to get the model, then creates and returns the response based on that model.
tests/test_wave_01.py
Outdated
| "is_complete": False, | ||
| "goal_id": None |
There was a problem hiding this comment.
Changing the set up data or assertions of an existing test will cause a project to be red until the test has been reverted to its original content. Folks are welcome to add their own new tests for waves if you think of edge cases you want to test for, but any existing test step up and assertions should be left unchanged.
All existing test set up data and assertions should be left as-is to ensure the tests are still meeting the requirements laid out in the project documentation.
- Changing either the initial data or the assertions means that the tests are no longer checking for the requirements laid out in the project description and do not guarantee correct behavior.
Please revert this test back to the original assertion when you are able and the grade can be moved up to a yellow or green.
2 participants
No description provided.