My_task_list_api #19
My_task_list_api #19
Conversation
There was a problem hiding this comment.
Overall good work on task-list-api. Your tests pass, I was able to run your migrations, and visit the routes you wrote.
In the future, I encourage you to practice making smaller more frequent commits. You could make a commit at the end of each wave, but some of these waves have a scope that's big enough that you could even make several commits for each wave.
Let me know if you have any questions about my comments.
| from .routes.task_routes import tasks_bp | ||
| from .routes.goal_routes import goals_bp |
There was a problem hiding this comment.
We prefer to name the task_bp and goal_bp just bp in their respective route files, and then either do an import as here to differentiate them, or import each entire route module and access its Blueprint using dot notation.
| from .routes.task_routes import tasks_bp | |
| from .routes.goal_routes import goals_bp | |
| from .routes.task_routes import bp as tasks_bp | |
| from .routes.goal_routes import bp as goals_bp |
| app.register_blueprint(tasks_bp, url_prefix="/tasks") | ||
| app.register_blueprint(goals_bp, url_prefix="/goals") |
There was a problem hiding this comment.
| app.register_blueprint(tasks_bp, url_prefix="/tasks") | |
| app.register_blueprint(goals_bp, url_prefix="/goals") | |
| app.register_blueprint(tasks_bp) | |
| app.register_blueprint(goals_bp) |
We don't need to add url_prefix here since we do it in the route files (see goal_routes.py line 6 and task_routes.py line 8).
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| __tablename__ = "goals" | ||
|
|
||
| id: Mapped[int] = mapped_column(Integer, primary_key=True, autoincrement=True) |
There was a problem hiding this comment.
Nitpick: you have extra whitespaces here. Before you open a PR for code review at internship be sure that your code is cleaned up. Code that doesn't follow convention won't be reviewed or allowed to be merged because the team won't want to introduce inconsistencies into a codebase because it makes it more difficult to maintain and read.
| id: Mapped[int] = mapped_column(Integer, primary_key=True, autoincrement=True) | |
| id: Mapped[int] = mapped_column(Integer, primary_key=True, autoincrement=True) |
There was a problem hiding this comment.
What happened to the white spacing in this file? 🧐
Is this intentional or did this formatting get brought in with the help of AI?
I find the whitespaces like this make it challenging to read the code and it would be even more challenging to maintain a codebase like this. I prefer to not have the extra whitespaces and for the code to follow the guidelines outlined in the PEP8 style guide (linked above)
|
|
||
| class Goal(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| __tablename__ = "goals" |
There was a problem hiding this comment.
Unless a team has a convention of preferring to explicitly name a table in the plural form, we should be ok to just leave the table name the same as the class name, which means line 6 can be removed.
| if task.goal_id is not None: | ||
| task_data["goal_id"] = task.goal_id |
There was a problem hiding this comment.
It would be nice if this logic was part of to_dict in task.py so that the code in this route can be more concise and single-responsibility.
| __tablename__ = "goals" | ||
|
|
||
| id: Mapped[int] = mapped_column(Integer, primary_key=True, autoincrement=True) | ||
| title: Mapped[str] = mapped_column(String, nullable=False) |
There was a problem hiding this comment.
In the current version of SQLAlchemy nullable=False is the default so we should leave off mapped with nullable because this adds more code to the codebase that isn't needed.
| title: Mapped[str] = mapped_column(String, nullable=False) | |
| title: Mapped[str] |
|
|
||
| def to_dict(self) -> dict: | ||
| return { | ||
| "id": self.id, |
There was a problem hiding this comment.
Nitpick: fix up whitespaces here. Please review the PEP8 style guide here on how to use whitespaces in your code if this topic feels unclear.
| "id": self.id, | |
| "id": self.id, |
| id: Mapped[int] = mapped_column(Integer, primary_key=True, autoincrement=True) | ||
| title: Mapped[str] = mapped_column(String, nullable=False) | ||
| description: Mapped[str] = mapped_column(Text, nullable=False) | ||
| completed_at: Mapped[DateTime] = mapped_column(DateTime, nullable=True) |
There was a problem hiding this comment.
Another option, which is the one we show in our curriculum and also shown in the SQLAlchemy documentation is Optional[].
To set nullability for a table's column we can use Optional[] per the SQLAlchemy documentation. We do not need to add more specific typing information regarding nullability so we don't need to use mapped_column.
You can review Building an API Part 3 to read about the Optional syntax too.
Therefore, these two lines of code accomplish the same thing:
completed_at: Mapped[DateTime|None]
completed_at: Mapped[Optional[DateTime]]|
|
||
| def validate_goal(goal_id): | ||
| try: | ||
| gid = int(goal_id) |
There was a problem hiding this comment.
We can reassign the value of goal_id here instead of creating a new variable.
I'd prefer reassignment because gid is concise but not as descriptive as goal_id. We also do not need goal_id to continue referencing the string version of this number so it's safe to reassign the variable to be the int value.
| gid = int(goal_id) | |
| goal_id = int(goal_id) |
We do the same thing here in Flasky.
model_id = int(model_id)2 participants
No description provided.