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 - BRITTANY C. #54

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

ROCK - BRITTANY C. #54

wants to merge 9 commits into from

Conversation

brittanygcle
Copy link

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.

This looks very nice, Brittany! Some things you could consider for a future project is that functions should only do one thing. That means we could separate out each request method into its own route decorator and function, too.

Also, keep blank lines to a minimum. Sometimes spacing out chunks of code will help, but doing it too much can make it hard to read!

Comment on lines 6 to +7
from dotenv import load_dotenv
import datetime

Choose a reason for hiding this comment

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

since you aren't using datetime in this file, we don't need to import it. also, are you calling load_dotenv anywhere? were your environment variables able to load without it?

Suggested change
from dotenv import load_dotenv
import datetime
from dotenv import load_dotenv
load_dotenv()

@@ -1,6 +1,33 @@
from types import new_class

Choose a reason for hiding this comment

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

is this being used?

Suggested change
from types import new_class

from flask import current_app
from app import db
from app.models.task import Task

Choose a reason for hiding this comment

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

Suggested change
from app.models.task import Task

"title": self.title
}

def goal_json_object_with_tasks(self):

Choose a reason for hiding this comment

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

👍 great helper method!

@@ -1,6 +1,32 @@
from flask import current_app
from app import db

from datetime import datetime

Choose a reason for hiding this comment

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

Suggested change
from datetime import datetime

Comment on lines +101 to +104
task = Task.query.get(task_id)

if task is 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.

here we could use get_or_404() method, too!

Comment on lines +120 to +132

if request.method == "PATCH":

task = Task.query.get(task_id)

if task is None:
return jsonify(None), 404

task.completed_at = None

return jsonify({"task":task.json_object()}),200


Choose a reason for hiding this comment

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

careful with adding too many blank lines! it can make code harder to read

Suggested change
if request.method == "PATCH":
task = Task.query.get(task_id)
if task is None:
return jsonify(None), 404
task.completed_at = None
return jsonify({"task":task.json_object()}),200
if request.method == "PATCH":
task = Task.query.get(task_id)
if task is None:
return jsonify(None), 404
task.completed_at = None
return jsonify({"task":task.json_object()}),200

Comment on lines +136 to +139
goal = Goal.query.get(goal_id)

if goal is 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.

here we could use get_or_404() method, too!

Comment on lines +185 to +197
def goals_task(goal_id):

form_data = request.get_json()

for task_id in form_data["task_ids"]:

task = Task.query.get(task_id)

task.goal_id = goal_id

db.session.commit()

return make_response({"id": int(goal_id),"task_ids":form_data["task_ids"]}), 200

Choose a reason for hiding this comment

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

try not to use so many blank lines

Comment on lines +202 to +204
goal = Goal.query.get(goal_id)
if goal is None:
return make_response("", 404)

Choose a reason for hiding this comment

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

here we could use get_or_404() method, 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