Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Nice job!
Your tests are all passing and your code is very readable. Great job thinking about how to separate the various responsibilities of your code. Deciding where things should go is really tricky, and may often require making up new types (not Models, just Python types) to hold a particular responsibility (SRP). Keep at it!
Overall, looks good!
| @@ -0,0 +1 @@ | |||
| web: gunicorn 'app:create_app()' No newline at end of file | |||
There was a problem hiding this comment.
You have the Procfile required for deployment, but didn't do the deployment?
| if test_config is None: | ||
| app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get( | ||
| if not test_config: | ||
| app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False |
There was a problem hiding this comment.
This and line 22 are the same in both branches, so it it could be moved outside the conditions (it used to be line 15).
| # def create_app(test_config=None): | ||
| # app = Flask(__name__) | ||
| # app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False | ||
|
|
||
| # if test_config is 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") | ||
| # print("SQLALCHEMY_DATABASE_URI is " + app.config["SQLALCHEMY_DATABASE_URI"]); | ||
| # # Import models here for Alembic setup | ||
| # from app.models.tasks import Task | ||
| # from app.models.goal import Goal | ||
|
|
||
| # db.init_app(app) | ||
| # migrate.init_app(app, db) | ||
|
|
||
| # # Register Blueprints here | ||
| # from .routes import tasks_bp | ||
| # app.register_blueprint(tasks_bp) | ||
|
|
||
| # return app |
There was a problem hiding this comment.
What's this code doing here? It looks almost the same, but doesn't have goals. Try to clean up things like this before submitting, or add some other label for why it's here. Even better, practice using branches and move code that you'd like to hang onto for reference to a side branch.
| @@ -1 +1,352 @@ | |||
| from flask import Blueprint No newline at end of file | |||
|
|
|||
| from wsgiref.util import request_uri | |||
| @@ -1 +1,352 @@ | |||
| from flask import Blueprint No newline at end of file | |||
|
|
|||
There was a problem hiding this comment.
It looks like you were thinking about moving the goal routes to their own file, which is a great idea. But since everything stayed in this routes.py, I'd remove the empty goal_routes.py.
| description = db.Column(db.String, nullable=False) | ||
| completed_at = db.Column(db.DateTime) | ||
| goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id'), nullable=True) | ||
| goals = db.relationship("Goal") |
There was a problem hiding this comment.
Since you used backref in Goal, we don't need this relationship here. If you do want to have both entries in both places, prefer back_populates.
| if self.goal_id: | ||
| hash_map["goal_id"] = self.goal_id |
There was a problem hiding this comment.
👍 Nice way to selectively add the goal id key for tasks that have one.
| @goals_bp.route('/<goal_id>/tasks', methods=['GET']) | ||
| def get_task_for_goal(goal_id): | ||
| goal = validate_element(goal_id, "goal") | ||
| tasks = Task.query.filter_by(goal=goal) |
There was a problem hiding this comment.
This works, but since you set up relationships, you could also do
tasks = goal.tasks| task_list = [] | ||
|
|
||
| for task in tasks: | ||
| task_list.append(task.to_json()) |
|
|
||
|
|
||
|
|
||
| # def error_message(message, status_code): |
There was a problem hiding this comment.
There's 150 lines of commented code after this. Be sure to clean that up from a submission, such as by moving it to a side branch.
2 participants
No description provided.