Accelerate - Dena Burd #5
Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Great job!
All your tests are passing and your code is very readable. I also liked the added touch of having the route documentation available at the root endpoint. Very classy!
One main thing I'd focus on going forward is identifying repetitive parts of the code and moving them to helper methods or class instance methods. Especially around creating CRUD models from some request body, and outputting the models as JSON. We can create our own additional classes to help us organize common code and represent reusable ideas. Even if the output isn't exactly the same, thinking about how to move that similar code into shared routines can help up organize and test our code.
Our testing in this project focused on testing through the flask client helper, but we can write smaller unit tests similar to the tests we used in Video Party and Swap Meet, which are easier to write when the logic is pulled out into small units of code that focus on single responsibilities.
Definitely keep digging through the documentation for SqlAlchemy and Flask, and see what other helpers and patterns you can run across. Looking for those patterns helps us identify opportunities for code reuse and while it can be harder to build initially, it can leave us with less code overall and fewer opportunities for errors.
Overall, really nice job! 🎉
| @@ -0,0 +1,30 @@ | |||
| """empty message | |||
There was a problem hiding this comment.
Nice to see that your migrations appear to have remained intact over the course of the project. We've been seeing things go astray more than I would have expected where folx have simply needed to drop everything and regenerate their migrations.
| @@ -0,0 +1,148 @@ | |||
| <!DOCTYPE html> | |||
There was a problem hiding this comment.
This is a really nice piece of information you added. Did you need to hand code this?
And I noticed there are references to an Index.css and Index.js that aren't included. Did you have plans to do more?
|
|
||
| def create_app(test_config=None): | ||
| app = Flask(__name__) | ||
| app.url_map.strict_slashes = False |
There was a problem hiding this comment.
Nice way to set this across all your blueprints!
| @app.route("/") | ||
| def index(): | ||
| return render_template("index.html") |
There was a problem hiding this comment.
Nice one-off route for your documentation page!
| tasks_bp = Blueprint("tasks", __name__, url_prefix="/tasks") | ||
| goals_bp = Blueprint("goals", __name__, url_prefix="/goals") | ||
|
|
||
| load_dotenv() |
There was a problem hiding this comment.
I'm curious why you decided to put the dotenv stuff here rather than in the app/__init__.py
| task.title = form_data["title"] | ||
| task.description = form_data["description"] | ||
| task.completed_at = form_data["completed_at"] |
There was a problem hiding this comment.
I mentioned this already above, but we should be sure that the same fields required for POSTing are included here for PUT. PUT replaces the value for the supplied task id, so we should ensure that all of the values required to represent a Task are provided with the same restrictions as we had when creating it in the first place.
| channel = "channel=task-notifications" | ||
| text = f"text=Someone just completed the task {task.title}" | ||
|
|
||
| path = f"https://slack.com/api/chat.postMessage?{channel}&{text}" |
There was a problem hiding this comment.
Rather than us being responsible for adding query params into the url directly, we can make use of the params named param on the call to requests.post. Doing so would allow the requests library itself to do any necessary encoding of our data values, and we get to work with a nice Python dictionary.
Even better, since this is a post request, we can send the parameters in the post body using the data named parameter (similar to when we pass things as json but the way the values are packed into the request is a little different).
|
|
||
|
|
||
| @goals_bp.route("/", methods=["GET", "POST"]) | ||
| def handle_goals(): |
There was a problem hiding this comment.
Similar feedback about splitting these functions, and moving validation and dictionary-handling logic around that I made for Task will also apply to Goals. These are common themes for any model with CRUD operations.
| # if not task: | ||
| # return make_response(f"Task #{task_id} Not Found", 404) |
There was a problem hiding this comment.
Curious why you commented this out. We should check to make sure that each task id we lookup is valid.
| task = Task.query.get(task_id) | ||
| # if not task: | ||
| # return make_response(f"Task #{task_id} Not Found", 404) | ||
| task.goal_id = goal_id |
There was a problem hiding this comment.
I personally sort of like this style of setting the relationship, since this is really what is happening even if we use the sqlalchemy helpers. However, what would happen if the goal previously had some tasks set. Do we want to add the new tasks to the existing tasks? Do we want to replace them and sever any prior task → goal relationships? What behavior is implemented here?
2 participants
No description provided.