-
Notifications
You must be signed in to change notification settings - Fork 128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Kunzite - Erina P. #124
base: main
Are you sure you want to change the base?
Kunzite - Erina P. #124
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Just a few comments left below.
title = db.Column(db.String, nullable=False) | ||
tasks = db.relationship("Task", back_populates="goal", lazy=True) | ||
|
||
def to_dict(self, tasks=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works well and I like the default value approach!
task_dict["description"] = self.description | ||
task_dict["is_complete"] = self.completed_at != None | ||
|
||
if self.goal_id: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
return cls( | ||
title=task_dict["title"], | ||
description=task_dict["description"], | ||
completed_at=task_dict.get("completed_at") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good approach, setting competed_at
as None
if it wasn't passed to the function in the task_dict
|
||
goal_message = {"details": f'Goal {goal.goal_id} \"{goal.title}\" successfully deleted'} | ||
|
||
return jsonify(goal_message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how you've been using the fact that Flask's response default HTTP status code is 200 and therefore omitting it from the return
statements
task = validate_model(Task, task_id) | ||
task.goal_id = goal.goal_id | ||
|
||
goal.title = request.args.get("title", goal.title) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it appears that you're making it possible to send a request with a title
query string param and use that to potentially update the title
value for that goal (and if there's no such query string param, re-writing the value with the value already set). I think I'd avoid this pattern since it would not be expected that a POST request would update a resource (and overwriting a database's row's value with the same value is additional, albeit minor, overhead.
|
||
tasks_bp = Blueprint("tasks_bp", __name__, url_prefix="/tasks") | ||
|
||
def validate_model(cls, model_id): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a good opportunity for some refactoring! Since you're defining the same function in two route files, you could create a new file route_helpers.py
where that function is defined and then import it wherever you need!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to do this with a little more time! I think I'll go back and add it!
No description provided.