Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Great job!
All your tests are passing and your code is very readable.
One main thing I'd focus on going forward is identifying repetitive parts of the code and moving them to helper methods or class instance methods. We can create our own additional classes to help us organize common code and represent reusable ideas. Especially for CRUD models, there is a lot of very similar code. Even if it's not exactly the same, thinking about how to move that similar code into shared routines can help up organize and test our code. And in cases where you did added helpers (like task_dict and goal_dict) be sure to use them. Reusing helper functions improves out code quality since each time we call them rather than re-writing the code, we are using the same code that we have seen working elsewhere, giving us greater confidence in our code's correctness.
Our testing in this project focused on testing through the flask client helper, but we can write smaller unit tests similar to the tests we used in Video Party and Swap Meet, which are easier to write when the logic is pulled out into small units of code that focus on single responsibilities.
Specifically with respect to writing web apis with Flask and SqlAlchemy, keep looking at other examples and looking through the documentation for other ideas about helper methods and implementation patterns. Using decorator methods can also be a useful strategy for common tasks if we split our routes into separate methods.
Overall, you did really well, and the rest will come with continued practice and experimentation. Keep it up!!! 🎉
| from.routes import tasks_bp | ||
| app.register_blueprint(tasks_bp) | ||
|
|
||
| from.routes import goals_bp | ||
| app.register_blueprint(goals_bp) |
There was a problem hiding this comment.
👍
One thing we can do to help our routes file from getting too large is to consider making multiple files containing routes separated by the resource type. We might have a routes folder instead of a routes.py file, and inside that folder (along with a __init__.py) we could have a file per resource, so task.py and goal.py. Where each would have a blueprint and endpoints for that resource. When we have one blueprint per file, we often name the blueprint simply bp rather than including the resource name as part of it.
Then here, we could import and register the blueprints like
from .routes import task, goal
app.register_blueprint(task.bp)
app.register_blueprint(goal.bp)|
|
||
| class Goal(db.Model): | ||
| goal_id = db.Column(db.Integer, primary_key=True) | ||
| title = db.Column(db.String) |
There was a problem hiding this comment.
Should we be able to create a goal with a NULL title? Consider adding nullable=False here.
| def goal_dict(self): | ||
| return{ | ||
| "id":self.goal_id, | ||
| "title":self.title | ||
| } |
There was a problem hiding this comment.
👍
Nice helper to convert from a model instance to a dict for JSON results.
| @@ -1,6 +1,23 @@ | |||
| from flask import current_app | |||
| from flask import current_app, jsonify | |||
There was a problem hiding this comment.
Unless you are actually calling the jsonify method in the file (which I don't see) there's no need to import jsonify. Though come to think of it, I don't see current_app used here, so I don't know why the boilerplate code included that import at all! 😂
| @classmethod | ||
| def goal_arguments(cls, title_from_url): | ||
| if title_from_url: | ||
| goals = Goal.query.filter_by(title=title_from_url).all() | ||
| if not goals: | ||
| goals = Goal.query.filter(Goal.title.contains(title_from_url)) | ||
| else: | ||
| goals = Goal.query.all() | ||
| return goals No newline at end of file |
There was a problem hiding this comment.
Neat class helper to load goals with optional filter. One thing to be careful of is what we should always try to return consistent result type from methods, so if one path returns a list of results, we should generally try to return lists (even if they are empty do to no results) fo consistency. This can help the function caller write simpler code. Currently, if title_from_url is falsy, this function falls through and would return None. Returning None can be ok (though we should generally do it explicitly, otherwise it can look like we simply forgot to think about that case), but if we returned an empty list, then the caller could always simply iterate over any (possible no) results safely. But currently, they would first need to perform a falsy check first.
All that being said, I don't see this being used anywhere else in the code currently?
| for task_id in request_body["task_ids"]: | ||
| tasks_instances.append(Task.query.get(task_id)) | ||
|
|
||
| goal.tasks = tasks_instances |
There was a problem hiding this comment.
👍
Assigning to the tasks relationship is absolutely a valid way to associate tasks with a goal. We should also think about whether to interpret POSTing to this endpoint is setting the exact list of tasks that should be a part of the goal, or whether we are adding these tasks to the goal (in addition to anything that was already there). Neither the project description nor the test test_post_task_ids_to_goal_already_with_goals are entirely clear on the desired behavior. Which perspective does your implementation take? How could we implement the other approach?
| tasks_response.append({ | ||
| "id": task.task_id, | ||
| "goal_id": task.goal_id, | ||
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": bool(task.completed_at) | ||
| }) | ||
| response_body = { |
There was a problem hiding this comment.
Don't forget about task_dict. Helper methods get sad when they aren't used. 😀
| @@ -0,0 +1,39 @@ | |||
| """empty message | |||
There was a problem hiding this comment.
Don't forget to add a message to your migrations with -m when running the migration step.
| @@ -0,0 +1,24 @@ | |||
| """empty message | |||
There was a problem hiding this comment.
This looks like an extra, unnecessary migration. Having this around could confuse flask, since both this and the other migration look like "first" migrations (neither has a non-None down_revision value, which is how flask traces back the migration dependencies). So flask could theoretically pick either as the first, and you could end up with weird situations where a deployment won't properly generate the tables on the remote server, even if it's working locally.
Try to keep an eye out for stray migrations and keep them tidy.
| # assertion 2 goes here | ||
| assert response.status_code == 404 | ||
| assert response_body == None | ||
| assert Goal.query.all() == [] |
There was a problem hiding this comment.
An alternative to directly accessing the DB here would be to make an additional GET request. Doing so keeps us at the same level of abstraction (exercising the web API) while still checking that a change has been made (deletion of the goal) without assuming how the deletion is occuring (maybe we want to use a deleted flag rather than actually deleting the row).
2 participants
No description provided.