Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Congrats on your first solo project API! I've left some comments and questions across the PR, please take a look when you can and reply here or on Slack if there's anything I can clarify =]
| # @classmethod | ||
| # def from_dict(cls, task_data): | ||
| # return cls(title=task_data["title"], description=task_data["description"]) No newline at end of file |
There was a problem hiding this comment.
Gentle reminder that we want to clean up commented code before opening PRs - we can look up our past commits on GitHub if we need to take a look at a previous state =]
| db.session.delete(goal) | ||
| db.session.commit() | ||
|
|
||
| return make_response({"details": 'Goal 1 "Build a habit of going outside daily" successfully deleted'}, 200) |
There was a problem hiding this comment.
How might we split this line up to keep it under 79 characters? What other lines across the project would be nice to split up for readability?
|
|
||
|
|
There was a problem hiding this comment.
One of the things to look at when we are in our code clean up phase is whitespace across a file. The general guideline is one newline between functions inside of a class, 2 new lines for top level functions in a file - but what's most important is to be consistent across a codebase. A little extra whitespace is often used as a visual separation between groups of related functions, or to make it clear where imports end and implementation code begins. We lose the ability to have a visual separation have meaning if we are not consistent with how they are applied. The PEP 8 style guide has a little more info on their recommendations: https://peps.python.org/pep-0008/#blank-lines
There was a problem hiding this comment.
Since this file mostly holds task routes, I suggest updating the name to reflect that.
| "id": self.id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": self.completed_at != None |
There was a problem hiding this comment.
Great choice to derive the value for is_complete from completed_at!
I want to share a gentle reminder that we should use is or is not when comparing objects to None in python. Another option could be to use the python bool function, what could that look like?
| from app.routes.routes import tasks_bp | ||
| app.register_blueprint(tasks_bp) | ||
| from app.routes.goal_routes import goals_bp |
There was a problem hiding this comment.
Nice choice to split up the routes into files that are more specific to the resources they work with!
There was a problem hiding this comment.
Really nicely divided up routes overall, there a a few long lines, but the use of spacing and naming makes things easy to quickly read and understand =]
| # # Assert | ||
| # # ---- Complete Test ---- | ||
| assert response.status_code == 404 | ||
| assert response_body == {"message":f"goal 1 not found"} |
There was a problem hiding this comment.
Since we hard code the 1 in the response, we could take off the f and use "goal 1 not found".
2 participants
No description provided.