Conversation
apradoada
left a comment
apradoada
left a comment
There was a problem hiding this comment.
This looks so good, Izzy! I just added a few comments here or there but overall well done!
| app.register_blueprint(task_list_bp) | ||
| app.register_blueprint(goals_bp) | ||
|
|
||
| return app |
There was a problem hiding this comment.
Remember to make an empty init.py file in any package folder/subfolder. app has one, but we should have one here in the models folder as well.
| @classmethod | ||
| def from_dict(cls,request_data): | ||
| return cls( | ||
| title=request_data["title"]) No newline at end of file |
| task_id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
| title = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| completed_at = db.Column(db.DateTime, nullable=True) |
There was a problem hiding this comment.
The default for a nullable constraint is True, so you can absolutely leave that out here if you would like!
| "id": self.task_id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": self.completed_at != None} |
There was a problem hiding this comment.
Great job using the inclusion of the .completed_at to define the truthiness of is_complete! Just a slight nitpick, make sure to move the brace at the end to the next line!
|
|
||
|
|
||
|
|
||
| def validate_task(task_id): |
There was a problem hiding this comment.
Not too big of an issue since you are only ever using validate_task to validate your tasks, but it's never a bad idea to make it a generic function in case you have other models that need to be validated later! Also, since it is a helper function, best practice would either have it placed before your routes or in its own helper file!
| response_body = {"task":task.task_dict()} | ||
|
|
||
|
|
||
| return make_response(jsonify(response_body), 200) |
There was a problem hiding this comment.
We need to have error handling for a request that uses the request body to create a response (like you have in the POST route). If request_body doesn't have a key "title" then line 75 would throw an unhandled exception.
| task.completed_at=None | ||
| db.session.commit() | ||
|
|
||
| return jsonify({"task":task.task_dict()}), 200 |
| "title": goal.title, | ||
| "tasks" : tasks_response | ||
| } | ||
| return jsonify(response_body), 200 |
There was a problem hiding this comment.
While it is possible to attach multiple methods to a single route like you've done here, it starts to get a bit cluttered. It will absolutely depend on how your team wants to handle things, but overall, it's a good idea to separate each method out for readability!
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": bool(task.completed_at) | ||
| }) |
There was a problem hiding this comment.
This looks nearly identical to what you have in the instance method to_dict in the Task class. We should use the to_dict method on each task from goal.tasks instead of repeating code. You already have the logic to handle adding goal_id to each task dict too in to_dict!
| # Check that the goal was deleted | ||
| response = client.get("/goals/1") | ||
| assert response.status_code == 404 | ||
| assert response_body == {"message": "Goal 1 not found"} |
| from app.models.goal import Goal | ||
|
|
||
| # Register Blueprints here | ||
| from .routes import task_list_bp,goals_bp |
There was a problem hiding this comment.
Don't be afraid to separate your routes into task_routes and goal_routes!
2 participants
No description provided.