-
Notifications
You must be signed in to change notification settings - Fork 111
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
Sea Turtles - Shannon Bellemore - Task List API #116
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.
Great job Shannon! Your code was readable and organized. I left comments on a few things you can refactor! Let me know if you have any questions!
from .task_routes import tasks_bp | ||
app.register_blueprint(tasks_bp) | ||
|
||
return app | ||
from .goal_routes import goals_bp | ||
app.register_blueprint(goals_bp) |
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 that you organized tasks and goals into two separate files. You can also move them to a directory called routes
to make it even more organized.
goals_bp = Blueprint("goals_bp", __name__, url_prefix="/goals") | ||
|
||
# helper functions | ||
def validate_goal(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.
great validation and error handling. You can also move this function and other helper functions into a file helper_functions.py
def create_goal(): | ||
request_body = request.get_json() | ||
if "title" not in request_body: | ||
return make_response(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.
you can add more specific details Invalid Data: the title is missing
just like you did in your validate helper function
goal_id = db.Column(db.Integer, primary_key=True) | ||
goal_id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
title = db.Column(db.String) | ||
tasks = db.relationship("Task", back_populates="goal", lazy=True) |
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.
Because the default value for lazy is True you could technically leave it off. More info here https://docs.sqlalchemy.org/en/14/orm/loading_relationships.html
task_id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
title = db.Column(db.String, nullable=False) | ||
description = db.Column(db.String, nullable=False) | ||
completed_at = db.Column(db.DateTime) |
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.
you could add nullable=True here because completed_at doesn't have to exist
goal_id = db.Column(db.Integer, db.ForeignKey("goal.goal_id")) | ||
goal = db.relationship("Goal", back_populates="tasks") | ||
|
||
def make_dict(self): |
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.
Because this is repetitive you could create a response dictionary to check for goal_id something like
response_dict = dict(
id=self.task_id,
title=self.title,
description=self.description,
is_complete= bool(self.completed_at)
)
if self.goal_id:
response_dict["goal_id"] = self.goal_id
return response_dict
abort(make_response({"error":f"Task not found"}, 404)) | ||
return task | ||
|
||
def slack_bot(task): |
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.
🙌🏽
if "completed_at" in request_body: | ||
new_task = Task( | ||
title=request_body["title"], | ||
description=request_body["description"], | ||
completed_at=request_body["completed_at"]) | ||
else: | ||
new_task = Task( | ||
title=request_body["title"], | ||
description=request_body["description"]) |
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.
How could you use something like a ternary or conditional on line 47 instead of an if/else?
No description provided.