task list pull request #67
Conversation
There was a problem hiding this comment.
Awesome job Chan 🔥 You hit all the learning goals for this project and I can tell you took a lot of care with this project. Overall, your was neat and easy to read. You've got some clean functions that can be reduced even further using the .get_or_404() method. I'm glad you're seeing the value of helper methods/functions and are having fun with list comprehension and classes!
I added a few comments about how you could refactor and/or other ways of writing code.
Overall, great job and keep up the hard work 🔥 👍 ✨
| @@ -1,6 +1,21 @@ | |||
| from flask import current_app | |||
| from app import db | |||
| from sqlalchemy.orm import relationship | |||
There was a problem hiding this comment.
hmm.. I don't think you need this import from sqlalchemy as you're already importing and initializing it as db in __init__.py.
This means relationship is coming from db rather than sqlalhemy.orm in this file.
| "id": self.goal_id, | ||
| "title": self.title | ||
| } | ||
|
|
There was a problem hiding this comment.
Good job in setting up the one to many relationship here and using a helper method! 👍
| from sqlalchemy.orm import relationship | ||
| from sqlalchemy import ForeignKey |
There was a problem hiding this comment.
See comment about imports in goal.py
| def to_json(self): | ||
|
|
||
| task_dict = { | ||
| "id": self.task_id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": False if self.completed_at is None else True | ||
| } | ||
|
|
||
| if self.goal_id: | ||
| task_dict["goal_id"] = self.goal_id | ||
|
|
||
| return task_dict |
There was a problem hiding this comment.
Nice helper method here. This is a good way to adding in a goal_id only IF a non-null value is detected. The ternary here looks great as well. Another way you can write that is using:
| def to_json(self): | |
| task_dict = { | |
| "id": self.task_id, | |
| "title": self.title, | |
| "description": self.description, | |
| "is_complete": False if self.completed_at is None else True | |
| } | |
| if self.goal_id: | |
| task_dict["goal_id"] = self.goal_id | |
| return task_dict | |
| def to_json(self): | |
| task_dict = { | |
| "id": self.task_id, | |
| "title": self.title, | |
| "description": self.description, | |
| "is_complete": self.completed_at != None | |
| } | |
| if self.goal_id: | |
| task_dict["goal_id"] = self.goal_id | |
| return task_dict |
| task = Task.query.get(task_id) | ||
|
|
||
| if task == None: | ||
| return("", 404) |
There was a problem hiding this comment.
Nice!! You can also reduce these lines of code by using Flask's get_or_404() method
| task = Task.query.get(task_id) | |
| if task == None: | |
| return("", 404) | |
| task = Task.query.get_or_404(task_id) | |
There was a problem hiding this comment.
This method can be used pretty much anywhere you're grabbing an id.
| else: | ||
| task.completed_at = datetime.now() | ||
| db.session.commit() | ||
| slack_app.slack_bot_message(task) |
| if request.method == "GET": | ||
|
|
||
| tasks = Task.query.filter_by(goal_id=goal_id) | ||
| response_body = [task.to_json() for task in tasks] |
There was a problem hiding this comment.
Great use of a list comprehension here!
| @@ -0,0 +1,25 @@ | |||
| from app.models import task | |||
|
|
||
| if request.method == "GET": | ||
|
|
||
| tasks = Task.query.filter_by(goal_id=goal_id) |
There was a problem hiding this comment.
Ooo interesting how you didn't need to use a join here, only a filter by.
No description provided.