Philomena Kelly - Sharks #103
Conversation
There was a problem hiding this comment.
Nicely done, Philomena! One thing to consider is staying consistent as you can with your route responses. Sometimes you send through just a dictionary, sometimes you jsonify the dictionary, sometimes you bring in make_response, etc.
Double check your routes so that they are formatted consistently and that they have predictable behavior.
| completed_at = db.Column(db.DateTime, nullable=True) | ||
| is_complete = False | ||
| goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id')) | ||
| goal = db.relationship("Goal", back_populates="tasks") |
There was a problem hiding this comment.
great job! If you are curious, check out backref attribute in SQLAlchemy to save you a line of code here!
| def written_out(cls): | ||
| return "task" |
There was a problem hiding this comment.
I think this would make for a good @staticmethod over a @classmethod
| def written_out(cls): | |
| return "task" | |
| @staticmethod | |
| def written_out(): | |
| return "task" |
There was a problem hiding this comment.
also is this used anywhere? I couldn't find any files where this method was called? If that's the case, let's get rid of it
| goals_bp = Blueprint("goals_bp", __name__, url_prefix="/goals") |
There was a problem hiding this comment.
often in industry we will separate out routes based on models into their own file to help balance the workload on files, as well as making sure teammates don't step on each other's toes when coding
| response = { "goal": new_goal.to_json()} | ||
|
|
||
| return make_response(response, 201) |
There was a problem hiding this comment.
You are staying super consistent with your return statements with make_response(jsonify(), so let's bring in jsonify here as well
| task = validate_object("task", task) | ||
| task.goal_id = goal_id |
There was a problem hiding this comment.
this would make a good instance method
| PARAMS = {'text' : f"Someone just completed the task {task.title}", | ||
| 'channel' : 'task-notifications'} | ||
| HEADERS = {'Authorization' : f'Bearer {os.environ.get("TASKLIST_BOT_KEY")}'} | ||
| r = requests.get(url = URL, params = PARAMS, headers = HEADERS) |
There was a problem hiding this comment.
this would make a good helper function
|
|
||
|
|
||
|
|
| task.completed_at = None |
There was a problem hiding this comment.
we could make this into an instance method
| response = { "task": task.to_json()} | ||
|
|
||
| return response, 200 |
There was a problem hiding this comment.
keep consistency with your return statements here, so that users can use your API in a predictable fashion
| return response, 200 | |
| return make_response(jsonify(response), 200) |
|
|
||
| try: | ||
| object_id = int(object_id) | ||
| except: | ||
| abort(make_response({"message":f"{object_type} {object_id} invalid"}, 400)) | ||
|
|
||
| if object_type == "task": | ||
| item = Task.query.get(object_id) | ||
| elif object_type == "goal": | ||
| item = Goal.query.get(object_id) | ||
|
|
||
| if not item: | ||
| abort(make_response({"message":f"{object_type} {object_id} not found"}, 404)) | ||
|
|
||
| return item |
There was a problem hiding this comment.
Let's move this into its own file for helper functions
No description provided.