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

first commit #115

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

first commit #115

wants to merge 2 commits into from

Conversation

perugia33
Copy link

I will continue to try and resolve some fail tests

Copy link

@tgoslee tgoslee left a 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
Comment on lines 26 to 27
# from app.models.task import Task
# from app.models.goal import Goal
Copy link

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

Copy link
Author

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

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!

Comment on lines +45 to +50
response_body = { "goal":
{
"id": new_goal.goal_id,
"title": new_goal.title
}
}, 201
Copy link

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 ?

Copy link
Author

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


@goals_bp.route("/<goal_id>/tasks", methods=["POST"])

def post_list_of_ids(goal_id):
Copy link

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

Copy link
Author

Choose a reason for hiding this comment

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

completed

linked_task.goal_id = goal_id

db.session.commit()
print(goal.tasks)
Copy link

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

Comment on lines +33 to +35
title=request_body["title"],
description=request_body["description"])
Copy link

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

Copy link
Author

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)
Copy link

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

Copy link
Author

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"]
Copy link

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")
Copy link

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
Comment on lines 234 to 235
# }
Copy link

Choose a reason for hiding this comment

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

you can remove this.

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