Skip to content

Conversation

@SalmaAnany
Copy link

No description provided.

Copy link

@yangashley yangashley left a 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!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this directory used for?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created it thinking I could add all the database querying functions and make the code dry, but sadly, I did not have enough time.

from app.models.goal import Goal
from app.models.task import Task

goal_bp = Blueprint("goal_bp", __name__, url_prefix="/goals")

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_bp


class Goal(db.Model):
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
title: Mapped[str]

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default is not Nullable, right?

title: Mapped[str]
tasks = relationship('Task', back_populates='goal', cascade='all, delete-orphan')

def to_dict(self, noTasks=True):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use snake case for variables in python

Suggested change
def to_dict(self, noTasks=True):
def to_dict(self, no_tasks=True):

"goal_id": task.goal_id,
"title": task.title,
"description": task.description,
"is_complete": task.completed_at is not None})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also put this logic in a helper function like is_complete

Suggested change
"is_complete": task.completed_at is not None})
"is_complete": task.is_complete()})

task_text = f"Someone just marked the task '{task.title}' incomplete"

notification_messages = SlackMessage("task-notifications", task_text)
notification_was_sent = slack_client.post_message(notification_messages)
Copy link

@yangashley yangashley Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~~Recall that the project requirement was to use the Python package requests to make 3rd party API calls.

We want folks to get practice using requests and not use the SlackClient because not all 3rd party APIs wil provide you with a custom client.

@SalmaAnany Please let me know, as a repy to this comment, how you would re-write the code for calling Slack using requests.~~

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used requests, please check the client, that's my own class using requests.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I meant to remove this comment after I saw your SlackClient class! Thanks for pointing me there again.



def change_task_status(task_id, is_completed):
task = Task.query.get(task_id)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The <model>.query syntax has been deprecated since SQLAlchemy 2.0 and should not be used for new development. This needs to removed to use the newer syntax:

  query = db.select(Task).where(Task.id == task_id)
  model = db.session.scalar(query)

See how we made a generic helper method for validating and fetching records from the DB in Flasky here on lines 10-11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants