Lilian Mora tigers #131
Lilian Mora tigers #131
Conversation
There was a problem hiding this comment.
Good job, Lily! The logic in your routes looks good, though there is plenty of code I think we can offload into other files to separate out responsibilities. This is a very common practice in industry.
Looks like you are missing Wave 4, which brings in the Slack API. I highly recommend finishing this up when you have time (perhaps during winter break).
The following are a few suggestions for future projects:
-
Consider splitting out your routes into separate files based on the models. This is common practice in industry, and is part of MVC design principles.
-
Think about placing your helper functions located in
routes.pyinto a separate file to adhere to common practices you will likely see in industry. -
We regularly create dictionaries from database data in the routes. This responsibility can be given to the model classes. In a perfect world, routes should only be responsible for calling the database and sending the data or response body to the client. Everything else should be separated into helper methods or functions. Doesn't always work out that way, but it's a good thing to keep in mind!
-
Now here is a tricky discussion: do we save some lines of duplicate code by placing several HTTP methods in under the same route function, or separate them all out in order to adhere to a function's single responsibility principle? Hard to say! It would really depend on how your team does it, but keep this in mind. There isn't one right way to separate out our endpoints.
| from flask import Flask | ||
| from flask_sqlalchemy import SQLAlchemy | ||
| from flask_migrate import Migrate | ||
| import os | ||
| from dotenv import load_dotenv | ||
| db = SQLAlchemy() | ||
| migrate = Migrate() | ||
| load_dotenv() | ||
|
|
||
|
|
||
| def create_app(test_config=None): | ||
| app = Flask(__name__) | ||
| app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False | ||
| if test_config == None: | ||
| app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get("SQLALCHEMY_DATABASE_URI") | ||
| else: | ||
| app.config["TESTING"] = True | ||
| app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get("SQLALCHEMY_TEST_DATABASE_URI") | ||
| from app.models.task import Task | ||
| from app.models.goal import Goal | ||
|
|
||
| db.init_app(app) | ||
| migrate.init_app(app, db) | ||
|
|
||
| from .routes import tasks_bp, goals_bp | ||
| from .routes import tasks_bp | ||
| from .routes import goals_bp | ||
| app.register_blueprint(tasks_bp) | ||
| app.register_blueprint(goals_bp) | ||
| return app |
There was a problem hiding this comment.
All this information stays in the root __init__.py file. Refer back to Flasky, Solar System API, and Book Review. This file should be completely empty.
|
|
||
|
|
||
| class Goal(db.Model): | ||
| goal_id = db.Column(db.Integer, primary_key=True) | ||
| title = db.Column(db.String) |
There was a problem hiding this comment.
See task.py for details about setting required attributes to nullable=False
|
|
||
|
|
||
| class Goal(db.Model): | ||
| goal_id = db.Column(db.Integer, primary_key=True) | ||
| title = db.Column(db.String) | ||
| tasks = db.relationship("Task", backref= "goal") |
There was a problem hiding this comment.
As much as I love backref 😭 , consider using back_populates from now on, as backref will soon be deprecated.
| "title":self.title | ||
| } | ||
| @classmethod | ||
| def goal_arguments(cls,title_from_url): |
There was a problem hiding this comment.
Maybe we can make this method name a little more description. A model might have several arguments that will do different things. This looks to be filtering data, so perhaps we can call it filter or filter_goals.
There was a problem hiding this comment.
I don't see this being used anywhere. We filter by ascending in one of the routes, that would be a perfect time to modify this and use it!
| title = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| completed_at = db.Column(db.DateTime, nullable = True) |
There was a problem hiding this comment.
It turns out that nullable=True is the default value for nullable. So all of the columns for Task here are currently marked as nullable. But should title or description be allowed to be NULL? (Does that make sense from a data standpoint?) Consider adding nullable=False to those columns.
The way the project emphasized that completed_at needs to accept NULL values may make it seem like we needed to explicitly call out that nullable should be True, but it turns out this is the default for nullable. Instead, we should think about the other data in our model and consider whether it makes sense for any of it to be NULL. If not, we can have the database help us protect against that happening!
| abort(404) | ||
| else: | ||
| task.completed_at = time_stamp |
There was a problem hiding this comment.
Now let's turn this into: task.completed_at = datetime.now()
| header= {"Authorization"} | ||
| post_body = requests.post(header) | ||
| except TypeError: | ||
| pass |
There was a problem hiding this comment.
I see you started implementing the slack bot API here. When you get time, maybe over winter break, I highly recommend trying to finish it. Working with third-party APIs like this one will be something you'll be doing during your capstone.
| for goal in goals: | ||
| goal_response.append({'id': goal.goal_id, 'title': goal.title}) |
There was a problem hiding this comment.
This would make a good helper method as well in the Goal model
| if goal == None: | ||
| abort(404) |
There was a problem hiding this comment.
Here is another place we can create a helper function. Let's validate the parameter passed in through the route to check if it's the right data type, exists in our db, etc. We should do this for bouth our models.
Here's a class resource we made about this from Flasky
| is_complete = task.completed_at != None | ||
| task_response.append({'id': task.task_id, | ||
| 'goal_id': task.goal_id, | ||
| 'title': task.title, | ||
| 'description': task.description, | ||
| 'is_complete': is_complete}) |
There was a problem hiding this comment.
Another good candidate for a helper method in our Task model
| alembic==1.5.4 | ||
| attrs==20.3.0 | ||
| autopep8==1.5.5 | ||
| blinker==1.4 | ||
| certifi==2020.12.5 | ||
| chardet==4.0.0 | ||
| click==7.1.2 | ||
| Flask==1.1.2 | ||
| Flask-Migrate==2.6.0 | ||
| Flask-SQLAlchemy==2.4.4 | ||
| alembic==1.8.1 | ||
| attrs==21.4.0 | ||
| click==8.1.3 | ||
| Flask==2.2.2 | ||
| Flask-Migrate==4.0.0 | ||
| Flask-SQLAlchemy==3.0.2 | ||
| gunicorn==20.1.0 | ||
| idna==2.10 | ||
| iniconfig==1.1.1 | ||
| itsdangerous==1.1.0 | ||
| Jinja2==2.11.3 | ||
| Mako==1.1.4 | ||
| MarkupSafe==1.1.1 | ||
| packaging==20.9 | ||
| pluggy==0.13.1 | ||
| psycopg2-binary==2.9.4 | ||
| py==1.10.0 | ||
| pycodestyle==2.6.0 | ||
| pyparsing==2.4.7 | ||
| itsdangerous==2.1.2 | ||
| Jinja2==3.1.2 | ||
| Mako==1.2.3 | ||
| MarkupSafe==2.1.1 | ||
| packaging==21.3 | ||
| pluggy==1.0.0 | ||
| py==1.11.0 | ||
| pyparsing==3.0.7 | ||
| pytest==7.1.1 | ||
| pytest-cov==2.12.1 | ||
| python-dateutil==2.8.1 | ||
| python-dotenv==0.15.0 | ||
| python-editor==1.0.4 | ||
| requests==2.25.1 | ||
| six==1.15.0 | ||
| SQLAlchemy==1.3.23 | ||
| toml==0.10.2 | ||
| urllib3==1.26.5 | ||
| Werkzeug==1.0.1 |
There was a problem hiding this comment.
Hmm I'm not sure what happened here, but somehow your list of packages got erased. I would recommend recommitting this file with the original list of packages
No description provided.