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

Rock - Abigail C #51

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

Rock - Abigail C #51

wants to merge 8 commits into from

Conversation

ChaAbby
Copy link

@ChaAbby ChaAbby commented May 13, 2021

No description provided.

Copy link

@spitsfire spitsfire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job Abby! One thing to consider is making all your return statements consistent. Everything could return jsonify() or return a dictionary or return make_response. Either options work, but it's good to pick just one. That way your endpoints will have consistency when making tests or being read by others!

very small thing, though!

Comment on lines +12 to +17
def to_json_goal(self):
# This method was created so that we do not have to write out the dictionary many times in the routes.py file.
return {
"id": self.goal_id,
"title": self.title,
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nice helper method

Comment on lines +7 to +12
__tablename__ = 'task'
task_id = db.Column(db.Integer, primary_key=True, autoincrement=True)
title = db.Column(db.String)
description = db.Column(db.String)
completed_at = db.Column(db.DateTime, nullable=True)
goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id'), nullable = True)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id'), nullable = True)


def to_json(self):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 glad you put all your logic in the helper method instead of in the route!

Comment on lines +24 to +39
if self.goal_id == None:
return {
"id": self.task_id,
"title": self.title,
"description": self.description,
"is_complete": result
}
else:
return {
"id": self.task_id,
"title": self.title,
"description": self.description,
"is_complete": result,
"goal_id": self.goal_id

}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing we could do to shorten this is create the data structure first, then change out the key-value pair you need to.

Suggested change
if self.goal_id == None:
return {
"id": self.task_id,
"title": self.title,
"description": self.description,
"is_complete": result
}
else:
return {
"id": self.task_id,
"title": self.title,
"description": self.description,
"is_complete": result,
"goal_id": self.goal_id
}
task = {
"id": self.task_id,
"title": self.title,
"description": self.description,
"is_complete": result
}
if self.goal_id:
task["goal_id"] = self.goal_id
return task

Comment on lines +14 to +17
task = Task.query.get(task_id)
# With the GET, POST and DELETE request if there is nothing we output this
if request == None or task == None:
return jsonify(None), 404

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this works great! One thing we could do to shorten this is:

Suggested change
task = Task.query.get(task_id)
# With the GET, POST and DELETE request if there is nothing we output this
if request == None or task == None:
return jsonify(None), 404
task = Task.query.get_or_404(task_id)

this will do your 404 check for you!

Comment on lines +122 to +125
from app.models.goal import Goal
from flask import Blueprint, request, jsonify

goals_bp = Blueprint("goals", __name__, url_prefix="/goals")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could add Goal import to the top. also, we've already imported Blueprint, request, and jsonify, so we don't need to do it again.

Comment on lines +129 to +139
try:
# This portion is the POST request
request_body = request.get_json()
new_goal = Goal(title=request_body["title"])

db.session.add(new_goal)
db.session.commit()
return {"goal": new_goal.to_json_goal()}, 201
except KeyError:
return {
"details": "Invalid data"}, 400

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool! good job experimenting with a try-except statement

Comment on lines +143 to +145
goals = Goal.query.all()
if goals == None:
return []

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can let all() return an empty list for us

Comment on lines +155 to +158
goal = Goal.query.get(goal_id)
# With the GET, POST and DELETE request if there is nothing we output this
if request == None or goal == None:
return jsonify(None), 404

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could also use the get_or_404() method here, too!

Comment on lines +192 to +194
goal = Goal.query.get(goal_id)
if goal == None:
return jsonify(None), 404

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could also use the get_or_404() method here, too!

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