C22 - Sphinx - Tatiana Trofimova #38
Conversation
| from .routes.task_routes import bp as task_bp | ||
| from .routes.goal_routes import bp as goal_bp |
|
|
||
| @classmethod | ||
| def from_dict(cls, data): | ||
| return Goal(title=data["title"]) |
There was a problem hiding this comment.
While we can explicitly use Goal here, we can also use the cls keyword, like:
| return Goal(title=data["title"]) | |
| return cls(title=data["title"]) |
| from ..db import db | ||
|
|
||
| class Goal(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] = mapped_column(String(50)) |
There was a problem hiding this comment.
Consider whether the this column for Goal should be nullable. It feels odd to allow someone to create a goal that gets saved to the DB without a title.
| from ..db import db | ||
|
|
||
| class Task(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] = mapped_column(String(50)) |
There was a problem hiding this comment.
As with Goal, consider which of the columns for Task should be nullable and which should not be.
| task = Task( | ||
| title=data["title"], | ||
| description=data["description"], | ||
| completed_at=data["completed_at"], | ||
| ) | ||
|
|
||
| return task |
There was a problem hiding this comment.
Since you don't reference the variable task after you initialize it and before you return it, you can just directly return the instance of task without making a variable:
| task = Task( | |
| title=data["title"], | |
| description=data["description"], | |
| completed_at=data["completed_at"], | |
| ) | |
| return task | |
| return cls( | |
| title=data["title"], | |
| description=data["description"], | |
| completed_at=data["completed_at"], | |
| ) |
| @bp.post("/", strict_slashes=False) | ||
| def create_goal(): | ||
| return create_class_instance(Goal, request, ["title"]) |
There was a problem hiding this comment.
The logic in create_class_instance doesn't access the list of required fields so I think ["title"] should be removed since we don't need to pass it to the helper function
| @bp.post("/", strict_slashes=False) | ||
| def create_task(): | ||
| return create_class_instance(Task, request, ["title", "description"]) |
There was a problem hiding this comment.
Same comment as in goal_routes.py: looks like create_class_instance never accesses the last param required_fields so it should be removed
| return create_class_instance(Task, request, ["title", "description"]) | |
| return create_class_instance(Task, request) |
| } | ||
| message_status = requests.post( | ||
| url="https://slack.com/api/chat.postMessage", |
There was a problem hiding this comment.
Prefer the URL to be referenced by a constant variable instead of having a string literal here.
URL = "https://slack.com/api/chat.postMessage"
url=URL, | headers={ | ||
| "Authorization": environ.get('SLACK_API_KEY'), | ||
| "Content-Type": "application/json" |
There was a problem hiding this comment.
When you pass your payload as json, like you do on line 63, Flask will auto set the Contnet-Type to JSON so you don't need to
| # "channel": "U07GC9C8Y4X", # My Slack account ID | ||
| "username": "Task List app", |
There was a problem hiding this comment.
Prefer the string literals on lines 56 and 58 be referenced with constant variables like CHANNEL_NAME and USERNAME
fixing notofocations in deployed BE fixing notofocations in deployed BE fixing notofocations in deployed BE fixing notofocations in deployed BE
No description provided.