-
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
first commit #115
base: master
Are you sure you want to change the base?
first commit #115
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.
Good job Theresa. I added some comments on some ways you could refactor. Two tests in wave 3 and a few tests in Wave 6 seem to not be passing. Let's take a look in our 1:1.
app/__init__.py
Outdated
# from app.models.task import Task | ||
# from app.models.goal import Goal |
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 remove these comments
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've got internet again. Removed lines 26 and 27
goals_bp = Blueprint("goals_bp", __name__, url_prefix="/goals") | ||
|
||
# Helper Function | ||
def validate_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.
like that you have specific error messages so the user knows exactly wants going on!
response_body = { "goal": | ||
{ | ||
"id": new_goal.goal_id, | ||
"title": new_goal.title | ||
} | ||
}, 201 |
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 can you create an instance method or helper function so you don't have to repeat this code ?
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 can create an instance method but am unclear as to how to incorporate it in this function
app/goal_routes.py
Outdated
|
||
@goals_bp.route("/<goal_id>/tasks", methods=["POST"]) | ||
|
||
def post_list_of_ids(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.
I would make this function name more specific. Something like post_tasks_for_goal
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.
completed
app/goal_routes.py
Outdated
linked_task.goal_id = goal_id | ||
|
||
db.session.commit() | ||
print(goal.tasks) |
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.
remove print statements unless its beneficial for the user
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.
you could create an instance method in your Task model to handle this
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.
Same as prior comment. Not clear as to how to introduce in Function
|
||
return response_body | ||
# return make_response(jsonify(response_body), 201) |
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 want to keep this line. It is best practice to use jsonfiy and return a code like 201
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.
Code 201 was included in the response body. There was an error when I used line 55 and therefore switched to this method
task.title = request_body["title"] | ||
task.description = request_body["description"] | ||
# task.completed_at = request_body["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.
make sure to remove commented code that isn't explaining how your code is working
|
||
|
||
tasks_bp = Blueprint("tasks", __name__, url_prefix="/tasks") |
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 file is specific to tasks you could rename it to task_routes.py
and you could move both of your routes to a routes directory to organize it a little better.
app/routes.py
Outdated
# } |
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 remove this.
I will continue to try and resolve some fail tests