Skip to content
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

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Lia Gaetano task-list #75

wants to merge 29 commits into from

Conversation

lgaetano
Copy link

@lgaetano lgaetano commented Nov 5, 2021

No description provided.

Copy link

@jbieniosek jbieniosek left a 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):

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()

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:

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.

Comment on lines +48 to +51
mandatory_fields = ["title", "description", "completed_at"]
for field in mandatory_fields:
if field not in form_data:
return jsonify({"details": "Invalid data"}), 400

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):

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

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:

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants