Conversation
| from .routes.task_routes import tasks_bp | ||
| from .routes.goal_routes import goals_bp |
There was a problem hiding this comment.
To be more consistent with Flask and API patterns, we should name our blueprints bp, and in other files like __init__.py here we should import them with more specific names if necessary.
|
|
||
| class Goal(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] = mapped_column(nullable=False) |
There was a problem hiding this comment.
nullable=False is the default value for an attribute in SQLAlchemy. It is more common to leave off the mapped_column content unless we are changing a setting to something other than the default value.
class Goal(db.Model):
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
title: Mapped[str]
tasks: Mapped[list["Task"]] = relationship(back_populates="goal")| description: Mapped[str] = mapped_column(nullable=False) | ||
| completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True) | ||
| goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id")) | ||
| goal: Mapped[Optional["Goal"]] = db.relationship("Goal", back_populates="tasks") |
There was a problem hiding this comment.
We should be consistent with the syntax we use across a project to avoid confusion.
Since in Goal.py we use the shorthand relationship() function to create the convenience attribute tasks, it would make our code more concise and consistent to use the same syntax for the goal relationship here in Task.py.
What would the updated attribute look like with that change?
| description = task_data["description"] | ||
| completed_at = task_data.get("completed_at", None) | ||
| goal_id = task_data.get("goal_id", None) | ||
| return cls(title=title, description=description, completed_at=completed_at, goal_id=goal_id) |
There was a problem hiding this comment.
How can we break up this line to keep within PEP8 best practices of 79 characters or less?
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] = mapped_column(nullable=False) | ||
| description: Mapped[str] = mapped_column(nullable=False) | ||
| completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True) |
There was a problem hiding this comment.
Take a look at the the feedback around nullable for attributes on the Goal class.
- What attributes here on the
Taskclass does this feedback also apply to? - What would it look like to update those attributes?
| completed_at = task_data.get("completed_at", None) | ||
| goal_id = task_data.get("goal_id", None) |
There was a problem hiding this comment.
Great use of get for the optional values.
| goal.tasks = [] | ||
|
|
||
| for id in request_body["task_ids"]: | ||
| task = validate_model(Task, id) | ||
| task.goal_id = goal.id |
There was a problem hiding this comment.
This works, but we can also associate the tasks with a goal by assigning them to the goal.tasks list. A slightly more pythonic way we could do this is by using a list comprehension to gather the new tasks then overwriting goal.tasks:
task_ids = request_body["task_ids"]
new_tasks = [validate_model(Task, id) for id in task_ids]
goal.tasks = new_tasks| @@ -0,0 +1,32 @@ | |||
| """empty message | |||
There was a problem hiding this comment.
I am seeing a lot of empty messages in the migration files. Just like with our git commits we should be leaving a descriptive message of changes every time we migrate to help other developers easily be able to track the changes to the database structure over time.
| def create_model(cls, model_data): | ||
| try: | ||
| new_model = cls.from_dict(model_data) | ||
|
|
||
| except KeyError: | ||
| response = {'details': f'Invalid data'} | ||
| abort(make_response(response, 400)) | ||
|
|
||
| db.session.add(new_model) | ||
| db.session.commit() | ||
|
|
||
| return new_model.to_dict(), 201 |
There was a problem hiding this comment.
Since this both creates a model and creates an http response, I'd consider renaming this to create_model_and_response. Or to better follow single responsibility principles, we could split this into 2 functions:
- one function that only manages creating and returns the new model
- one function that uses the function above to get the model, then creates and returns the response based on that model.
2 participants
No description provided.