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

Finished Waves 1-6 #118

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

Finished Waves 1-6 #118

wants to merge 4 commits into from

Conversation

GitHub4Gigi
Copy link

No description provided.

Copy link

@yangashley yangashley left a comment

Choose a reason for hiding this comment

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

Nice work, Gricelda!

I left some comments about how you could refactor your code to use list comprehension and return dictionaries directly without building them up beforehand which can help make your code more concise.

Your routes look good! I added a couple of comments where you can add error handling for the PUT requests in case clients send incorrect req bodies.

For now your Task List API is a 🟡 ! If you choose to refactor and add in Waves 4 and 7 then let me know and I can re-review your project.

Comment on lines +30 to +34
new_goal = cls(
title = request_body['title']

)
return new_goal

Choose a reason for hiding this comment

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

I think line 31 and the paren on line 33 needs to be tabbed over to the right one more time to match the opening bracket on line 30.

You can also return the class without instantiating it and saving it to a variable like:

return cls(
    title = request_body['title']
)

Comment on lines +14 to +17
if not self.completed_at:
is_complete = False
else:
is_complete = True

Choose a reason for hiding this comment

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

Another way you could write this is within the task_dict after line 23, like:

"is_complete": True if self.completed_at else False

Comment on lines +46 to +51
new_task = cls(
title = request_body['title'],
description = request_body['description'],
completed_at = request_body.get('completed_at', None)

)

Choose a reason for hiding this comment

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

Lines 47 -51 needs to be indented one level deeper.

@@ -1,5 +1,37 @@
from app import db

from flask import jsonify, abort, make_response

class Goal(db.Model):

Choose a reason for hiding this comment

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

👍

@@ -1,5 +1,56 @@
from app import db

from flask import jsonify, abort, make_response

class Task(db.Model):

Choose a reason for hiding this comment

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

Nicely done

@@ -0,0 +1,104 @@
from datetime import date
from urllib import response

Choose a reason for hiding this comment

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

Delete unused imports

@task_bp.route("", methods=["GET"])
def get_all_tasks():
task_response_body = []
# tasks = Task.query.all()

Choose a reason for hiding this comment

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

Remove unused code so it doesn't accidentally get uncommented and execute when we don't expect it to.

@task_bp.route("", methods=["POST"])
def create_task():
request_body = request.get_json()
print(request_body)

Choose a reason for hiding this comment

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

Remove debugging print statements when you're done with them

Comment on lines +49 to +53
one_task = Task.validate(id)
response_body = {}
response_body["task"] = one_task.to_json()

return jsonify(response_body), 200

Choose a reason for hiding this comment

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

To make this a 2 line method, you could write the code like this too:

one_task = Task.validate(id)
return jsonify({"task": one_task.to_json()}), 200

one_task = Task.validate(id)
request_body = request.get_json()

one_task.update(request_body)

Choose a reason for hiding this comment

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

We should add error handling here in case the client sends an incorrect request body so that we can handle a potential KeyError, like you did in your POST request on lines 33-36

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