Conversation
chimerror
left a comment
chimerror
left a comment
There was a problem hiding this comment.
Good Work!
While I made a few comments about some relatively minor repetition, naming, and style issues, this code correctly implements the specification, is cleanly written, and is very readable. And thus good enough for Green!
| "id": self.task_id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": bool(self.completed_at), |
There was a problem hiding this comment.
I like this use of bool here. There's a few different ways to convert completed_at but this one is probably the most succinct. Not that succinctness is always a virtue, especially when it impacts readability (which it does not in your case). But here it's nice.
| def format_task(self): | ||
| return { | ||
| "id": self.task_id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": bool(self.completed_at), | ||
| } | ||
|
|
||
| def format_task_goal(self): | ||
| return { | ||
| "id": self.task_id, | ||
| "goal_id": self.goal_id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": bool(self.completed_at), | ||
| } |
There was a problem hiding this comment.
But this repetition between format_task_goal and format_task is not as nice. Rather than having two methods, keep in mind that keys (and their values) can be added to Python dictionaries after initialization. So you can do the adding of the "goal_id" key in an if block:
def format_task(self):
task_dict = {
"id": self.task_id,
"title": self.title,
"description": self.description,
"is_complete": bool(self.completed_at),
}
if self.goal_id:
task_dict["goal_id"] = self.goal_id
return task_dictand avoid the repetition.
| def slackbot_call(message): | ||
| PATH = "https://slack.com/api/chat.postMessage" | ||
| parameters = {"channel": "task-notifications", "text": message} | ||
|
|
||
| requests.post( | ||
| PATH, | ||
| params=parameters, | ||
| headers={"Authorization": os.environ.get("SLACKBOT_API_KEY")}, | ||
| ) |
| if not request_body.get("title") or not request_body.get("description"): | ||
| return {"details": "Invalid data"}, 400 |
There was a problem hiding this comment.
Good job using get here so you don't get the KeyError if you had used the indexing brackets (request_body["title"], for example).
| return make_response( | ||
| {"details": f'Task {task_id} "{task.title}" successfully deleted'} | ||
| ) |
There was a problem hiding this comment.
Not really a need for make_response here as we're not doing something like changing what the status code is. So we can just return this dictionary you've created as you do on line 95 above.
There was a problem hiding this comment.
I see a few other places where this is true, but I'll just point it out this once and not repeat myself.
| "task": { | ||
| "id": task.task_id, | ||
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": (bool(task.completed_at)), | ||
| } |
There was a problem hiding this comment.
I had a comment pointing out that format_task could have been used here, and while that's true, this could have been a downside to my suggestion that you combine format_task_goal when the Task returned contains a "goal_id" key (and thus possibly fail a test not expecting that key).
This could justify a separate format_task_goal... However, in most cases, we would still expect the "goal_id" key to be returned when doing this update. It's not so much that we have a reason to not show that key as part of the task, it's more that we didn't have that key during the earlier waves, but now we do.
Either way, I still think the cleanest way I would implement is still handling in my suggested format_task, as well as using it as a helper method here. In that way, format_task is defining what format we always return a task in, and that stays the same across endpoints.
There was a problem hiding this comment.
The use of format_task also applies to update_task_as_incomplete down below.
| goal.title = request_body["title"] | ||
| db.session.commit() | ||
|
|
||
| return make_response({"goal": {"id": goal.goal_id, "title": goal.title}}) |
There was a problem hiding this comment.
Like my comment on missed format_task opportunities, format_goal could have been used here.
There was a problem hiding this comment.
And other places, but I'll not repeat myself.
|
|
||
|
|
||
| @pytest.mark.skip(reason="No way to test this feature yet") | ||
| #@pytest.mark.skip(reason="No way to test this feature yet") |
There was a problem hiding this comment.
Minor style preference: Just delete these unneeded decorators rather than commenting them out. They are mainly there as scaffolding code, and now that the code works, we can remove them.
| def test_update_goal(client, one_goal): | ||
| raise Exception("Complete test") | ||
| # Act | ||
| # ---- Complete Act Here ---- | ||
| response = client.put("/goals/1", json={ | ||
| "title": "Updating my goals" | ||
| }) | ||
|
|
||
| response_body = response.get_json() | ||
|
|
||
| # Assert | ||
| # ---- Complete Assertions Here ---- | ||
| # assertion 1 goes here | ||
| assert response.status_code == 200 | ||
| # assertion 2 goes here | ||
| assert "goal" in response_body | ||
| # assertion 3 goes here | ||
| assert response_body == { | ||
| "goal": { | ||
| "id": 1, | ||
| "title": "Updating my goals", | ||
| } | ||
| } | ||
| # ---- Complete Assertions Here ---- | ||
| goal = Goal.query.get(1) | ||
| assert goal.title == "Updating my goals" |
| response_body = response.get_json() | ||
| assert response.status_code == 404 | ||
|
|
||
| raise Exception("Complete test with assertion about response body") | ||
| # ***************************************************************** | ||
| # **Complete test with assertion about response body*************** | ||
| # ***************************************************************** | ||
| assert response_body == {"error": "Item #1 cannot be found"} | ||
| assert Goal.query.all() == [] |
There was a problem hiding this comment.
This is also good test design, especially checking the DB and not just response.
2 participants
No description provided.