Task List Api #28
Task List Api #28
Conversation
| from .routes.task_routes import bp as task_bp | ||
| from .routes.goal_routes import bp as goal_bp |
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] | ||
| description: Mapped[str] | ||
| completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True) |
There was a problem hiding this comment.
Using Optional[] is all that we need to indicate that a field is nullable so we don't need to also include mapped_column with nullable=True (ilke how you did it for goal_id below on line 14).
| completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True) | |
| completed_at: Mapped[Optional[datetime]] |
| def is_complete(self): | ||
| return self.completed_at is not None |
There was a problem hiding this comment.
This logic is simple enough that you could get away with not using a helper function and using a ternary instead on line 22
| "id":self.id, | ||
| "title":self.title, | ||
| "description":self.description, | ||
| "is_complete": self.is_complete() |
There was a problem hiding this comment.
| "is_complete": self.is_complete() | |
| "is_complete": True if self.completed_at else False |
Instead of using a helper function, we could just use a ternary operator here.
|
|
||
| @bp.get("") | ||
| def get_goals(): | ||
| query=db.select(Goal) |
There was a problem hiding this comment.
Nitpick: spacing
| query=db.select(Goal) | |
| query = db.select(Goal) |
| @bp.get("/<id>") | ||
| def get_one_task(id): | ||
| task = validate_model(Task, id) | ||
| return {"task": task.to_dict()},200 |
There was a problem hiding this comment.
| return {"task": task.to_dict()},200 | |
| return {"task": task.to_dict()} |
| task.title=request_body["title"] | ||
| task.description=request_body["description"] |
There was a problem hiding this comment.
These two lines will throw unhandled exceptions if the request body does not have keys exactly as "title" or "description". How could you update the logic in this route so that, instead of throwing a server error, you could return a nice error response with an appropriate status code?
| data = { | ||
| "token": f"{os.environ.get('SLACK_API_TOKEN')}", | ||
| "channel":"test-slack-api", | ||
| "text":"Someone just completed the task My Beautiful Task" | ||
| } | ||
| response = requests.post("https://slack.com/api/chat.postMessage", data=data, | ||
| headers={ | ||
| "Authorization": f"Bearer {os.environ.get('SLACK_API_TOKEN')}" | ||
| }) |
There was a problem hiding this comment.
Prefer all this logic to live in a helper function, maybe like call_slack_api to make this route more single-responsibility and concise.
| "channel":"test-slack-api", | ||
| "text":"Someone just completed the task My Beautiful Task" | ||
| } | ||
| response = requests.post("https://slack.com/api/chat.postMessage", data=data, |
There was a problem hiding this comment.
Prefer not to have string literal hanging out in a function. We should use a constant variable to reference the URL and channel name.
We call a string literal that appears in code magic strings: "Magic Strings are literal string values that are used directly in code without a clear explanation...This makes it difficult to maintain and extend the code in the future."
Read more about why we should avoid magic strings here
SLACK_CHANNEL = "test-slack-api"
SLACK_URL = "https://slack.com/api/chat.postMessage"| task = validate_model(Task, id) | ||
|
|
||
| data = { | ||
| "token": f"{os.environ.get('SLACK_API_TOKEN')}", |
There was a problem hiding this comment.
We need to pass in the authorization token as a header, not as a key value pair in the json we send with the request. You do this on line 71 so we should remove the unnecessary code here.
| "token": f"{os.environ.get('SLACK_API_TOKEN')}", |
2 participants
No description provided.