Dragonfly Dennif Portilla #30
Conversation
| from app.models.task import Task | ||
| from app.models.goal import Goal |
There was a problem hiding this comment.
Notice that you import these two models, but don't reference them anywhere. We should remove unused imports so that our codebase doesn't become cluttered.
| from app.models.task import Task | |
| from app.models.goal import Goal |
| from dotenv import load_dotenv | ||
|
|
||
| def create_app(config=None): | ||
| load_dotenv() |
There was a problem hiding this comment.
| from dotenv import load_dotenv | |
| def create_app(config=None): | |
| load_dotenv() | |
| def create_app(config=None): |
We don't need load_dotenv() to get the environment variables when we start our Flask app with flask run. We only need to explicitly load our values from .env and have them be environment variables when we run our tests with pytest. That's why we only need to have load_dotenv() in conftest.py
| from .routes.task_routes import bp as tasks_bp | ||
| from .routes.goal_routes import bp as goals_bp |
| from typing import TYPE_CHECKING | ||
| if TYPE_CHECKING: from .task import Task |
There was a problem hiding this comment.
Nice inclusion of this extra check to clear up the type warnings in VS Code.
| new_goal = Goal(title=goal_data["title"]) | ||
| return new_goal |
There was a problem hiding this comment.
Since we have access to the class with the cls keyword that's necessary for defining a class method, we can use cls instead of Goal.
Using the more generic cls means that if we ever update the class name, we won't need to update line 15.
| new_goal = Goal(title=goal_data["title"]) | |
| return new_goal | |
| return cls(title=goal_data["title"]) |
| task.title = request_body["title"] | ||
| task.description = request_body["description"] |
There was a problem hiding this comment.
These two lines of code will throw unhandled exceptions if the request body does not have "title" key or "description" key.
How could you refactor this route so that you can send back a custom error response if the client sends an incorrect request body?
| slack_token = os.environ.get("SLACK_BOT_TOKEN") | ||
| channel = os.environ.get("SLACK_CHANNEL", "task-notifications") |
There was a problem hiding this comment.
These are constant values so we should name these variables accordingly.
| slack_token = os.environ.get("SLACK_BOT_TOKEN") | |
| channel = os.environ.get("SLACK_CHANNEL", "task-notifications") | |
| SLACK_TOKEN = os.environ.get("SLACK_BOT_TOKEN") | |
| CHANNEL = os.environ.get("SLACK_CHANNEL", "task-notifications") |
| if slack_token: | ||
| try: | ||
| requests.post( | ||
| "https://slack.com/api/chat.postMessage", |
There was a problem hiding this comment.
Prefer this string value to be referenced by a constant variable too
SLACK_URL = "https://slack.com/api/chat.postMessage"
requests.post(SLACK_URL,
# rest of your logic| slack_token = os.environ.get("SLACK_BOT_TOKEN") | ||
| channel = os.environ.get("SLACK_CHANNEL", "task-notifications") | ||
| if slack_token: | ||
| try: | ||
| requests.post( | ||
| "https://slack.com/api/chat.postMessage", | ||
| headers={ | ||
| "Authorization": f"Bearer {slack_token}", | ||
| "Content-Type": "application/json" | ||
| }, | ||
| json={ | ||
| "channel": channel, | ||
| "text": f"Someone just completed the task {task.title}" | ||
| } | ||
| ) | ||
| except Exception as e: | ||
| current_app.logger.error(f"Slack error: {e}") |
There was a problem hiding this comment.
Prefer all of this logic to be in a helper function (maybe like call_slack_api()). Using a helper function would make this route more concise and single-responsibility.
| assert response.status_code == 204 | ||
|
|
||
| # assertion 2: the response body is empty | ||
| assert response.data == b'' |
There was a problem hiding this comment.
Since we assert the status code is 204 and we know 204 means No Content, we could leave this assertion off.
2 participants
No description provided.