Conversation
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Great work on Task List, Huma! You earned a 🟢 on this project. Really nice to see you making frequent commits too.
From your code, I can see that you've got a nice handle on Flask and SqlAlchemy. Good to see you consistently using jsonify and I'd encourage you to reuse your instance method to_json() in your Goal and Task classes.
I added a few comments about using list comprehension and some refactors to help keep your routes concise.
Let me know if you have any questions!
| from app.models.task import Task | ||
| # datetime.datetime.utcnow() | ||
|
|
||
| goal_bp = Blueprint("goal_bp", __name__, url_prefix = "/goals") |
There was a problem hiding this comment.
To keep your project organized, you can create a routes directory under the app directory. In the routes directory, you can add goal_routes.py and you can rename routes.py to task_routes.py
| from flask import request | ||
| from datetime import datetime | ||
| from app.models.task import Task | ||
| # datetime.datetime.utcnow() |
There was a problem hiding this comment.
Deleting comments like this can keep your code clean
| goal_bp = Blueprint("goal_bp", __name__, url_prefix = "/goals") | ||
|
|
||
|
|
||
| def validate_goal(id_): |
There was a problem hiding this comment.
Curious about id_, is the underscore intentional here after id?
| try: | ||
| new_goal = Goal.create(request_body) | ||
|
|
||
| except KeyError: | ||
| return abort(make_response({"details": "Invalid data"}, 400)) |
There was a problem hiding this comment.
Nice error handling here! We can't assume that clients will send valid requests to our API so we have to anticipate a scenario where the req doesn't have the keys your method will need to access.
| for goal in goals: | ||
| goals_response.append( | ||
| { | ||
| "id": goal.id, | ||
| "title": goal.title | ||
| } | ||
| ) |
There was a problem hiding this comment.
If you use a to_json() method on each goal, then you could use list comprehension here to keep your route method concise. Something like:
goals_response = [goal.to_json() for goal in goals]| SLACK_BOT_POST_PATH = "https://slack.com/api/chat.postMessage" | ||
|
|
||
| query_params = { | ||
| "channel": "test-channel", | ||
| "text": f"Someone just completed the task {task.title}" | ||
| } | ||
|
|
||
| headers = {"Authorization": os.environ.get("SLACK_BOT_KEY")} | ||
|
|
||
| response_bot = requests.post(SLACK_BOT_POST_PATH, params=query_params, headers=headers) |
There was a problem hiding this comment.
Consider putting this into a helper method and then calling it here to keep the route concise. Using a helper method would also apply the SRP (single responsibility principle) to the route function.
| if sort_param == "desc": | ||
| tasks = Task.query.order_by(Task.title.desc()).all() | ||
| else: | ||
| # tasks = Task.query.all() | ||
| tasks = Task.query.order_by(Task.title.asc()).all() |
There was a problem hiding this comment.
with order_by() you don't need to use chain .all() to the end of this statement.
| for task in tasks: | ||
| task_response_body.append(task.to_json()) |
There was a problem hiding this comment.
You can use list comprehension here if you'd like.
|
|
||
| request_body = request.get_json() | ||
|
|
||
| task.update(request_body) |
There was a problem hiding this comment.
Consider adding some error handling here if request_body doesn't have "title" or "description"
| # Act | ||
| # ---- Complete Act Here ---- | ||
|
|
||
| response = client.put("/goals/1", json={ |
2 participants
No description provided.