-
Notifications
You must be signed in to change notification settings - Fork 97
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
Lia Gaetano task-list #75
base: master
Are you sure you want to change the base?
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.
Fantastic work on this project Lia! I really liked your require_instance_or_404 decorator, it streamlined the code significantly. This project is green!
"tasks": tasks | ||
} | ||
|
||
def update_from_dict(self, data): |
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.
Neat solution!
"id": self.id, | ||
"title": self.title, | ||
"description": self.description, | ||
"is_complete": self.check_if_completed() |
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.
👍
@require_instance_or_404 | ||
def get_task(task): | ||
"""Retrieve one stored task by id.""" | ||
if task.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.
This works but to simplify the code I would recommend pushing this check to the to_dict method and add the goal_id key-value pair if goal_id exists inside the to_dict function.
mandatory_fields = ["title", "description", "completed_at"] | ||
for field in mandatory_fields: | ||
if field not in form_data: | ||
return jsonify({"details": "Invalid data"}), 400 |
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!
from app.models.task import Task | ||
from app.models.goal import Goal | ||
|
||
def require_instance_or_404(endpoint): |
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.
💯 Fantastic!
return {"task": new_task.to_dict()}, 201 | ||
|
||
@tasks_bp.route("/<task_id>", methods=["PUT"]) | ||
@require_instance_or_404 |
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 work with this decorator! Setting up task
as the parameter in the function makes the code in the function very streamlined!
|
||
for task_id in form_data["task_ids"]: | ||
query = Task.query.get(task_id) | ||
if not query: |
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.
Very minor suggestion, query
is a little confusing here as a variable name as the value being stored is a task not a query, I'd recommend something like task
or current_task
.
No description provided.