-
Notifications
You must be signed in to change notification settings - Fork 44
task_list_api -Sunitha -Sphinx #34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
yangashley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on task-list-api!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before adding files to your commit history, review what files you have staged by running git status. Changes like the ones here that are not meaningful should not be a part of your pull request because that's essentially asking to merge in unnecessary changes to the codebase which can make the codebase messy.
| from ..db import db | ||
| from datetime import datetime | ||
|
|
||
| goals_bp = Blueprint("goals_bp", __name__, url_prefix="/goals") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Learn and in the livecode for Flasky, we name blueprints for each model bp. In goal_routes.py I can deduce that bp is the blueprint for Goal given the name of the file.
If we have multiple route files that each have a blueprint named bp, we can account for name collisions when importing them into app/__init__.py by using aliases:
from .routes.task_routes import bp as tasks_bp
from .routes.goal_routes import bp as goals_bpThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless these changes were required, I'd revert unnecessary changes so they don't get committed and reviewed when you open a PR.
|
|
||
| class Goal(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| def to_dict(self): | ||
| return dict(id=self.id, title=self.title) | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, goal_data): | ||
| return cls(title=goal_data["title"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| @goals_bp.get("/<goal_id>/tasks") | ||
| def get_tasks_by_goal(goal_id): | ||
| goal = validate_goal(goal_id) | ||
| goal_dict = goal.to_dict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we are creating a dictionary that represents a goal, it might be more descriptive to name this variable response since we're creating a dictionary that we want to send back as a response to the client.
| from datetime import datetime | ||
| from app.models.task import Task | ||
|
|
||
| tasks_bp = Blueprint("tasks_bp", __name__, url_prefix="/tasks") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above in goal_routes.py, could be named just bp
| request_body = request.get_json() | ||
|
|
||
| try: | ||
| new_task = Task.from_dict(request_body) | ||
|
|
||
| except KeyError: | ||
| response = {"details": "Invalid data"} | ||
| abort(make_response(response, 400)) | ||
|
|
||
|
|
||
| db.session.add(new_task) | ||
| db.session.commit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above in goal_routes.py, would prefer this logic to be in a helper function so you're not duplicating very similar code across different files.
| task = validate_task(task_id) | ||
|
|
||
|
|
||
| return {"task": task.to_dict()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| def mark_task_complete(task_id): | ||
| task = validate_task(task_id) | ||
| task.completed_at = datetime.now() | ||
| db.session.commit() | ||
|
|
||
| return {"task": task.to_dict()}, 200 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of time, I went to your repo and found your other branch to look at your Slack implementation. Note: I cannot leave comments on a codebase. That's why we have you open PRs for your projects. In the future, please make sure all of your solution is in your PR.
I'm copy and pasting your helper function here and leaving my review comments as comments in your code
def send_slack_message(task_title):
# constants should be named using all capital letters
token = os.environ.get("SLACK_BOT_TOKEN")
if not token:
raise ValueError("Slack Bot Token is not set in environment variables.")
headers = {"Authorization": f"Bearer {token}", "Content-Type": "application/json"}
payload = {"channel": SLACK_CHANNEL, "text": f"Beautiful task {task_title}"}
response = requests.post(SLACK_API_URL, headers=headers, json=payload)
if response.status_code != 200:
print("Failed to send Slack message:", response.json())
else:
print("Slack message sent successfully.")Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Please review.