Spruce: Asya and Gabe #23
Conversation
There was a problem hiding this comment.
Overall, this looks good! The main thing to consider going forward is looking for ways to move shared code into helper methods, and to reduce the complexity of the individual route functions by splitting them into their own functions, following the single responsibility principle.
Also, make sure that you are consistently having your endpoints return json. Messages and errors should be done carefully, to make sure that their responses are still building a response which is valid JSON.
| requests==2.25.1 | ||
| six==1.15.0 | ||
| SQLAlchemy==1.3.23 | ||
| alembic==1.7.4 |
There was a problem hiding this comment.
What prompted you to regenerate your requirements? There's nothing wrong with doing this (and in fact it gets you more up-to-date libraries), but it shouldn't have been strictly necessary.
| # Alternatively, we could do | ||
| # db.session.add(ocean_book) | ||
| # db.session.add(mountain_book) | ||
| db.session.commit() |
There was a problem hiding this comment.
Consider returning [nine_planet, pluto_planet] after this line, so that tests can make use of the ids of the records, and anything else that's part of the record that might be useful in the test.
|
|
||
| def test_get_one_planet(client, two_saved_planets): | ||
| # Act | ||
| response = client.get("/planets/1") |
There was a problem hiding this comment.
We could made this test a little less brittle, though perhaps a little tougher to read, if we write two_saved_planets to return the created models as well. We could then access the id of the model here, using the id the database gave the record. This can help us future-proof our tests (in case we change the number or order of records created, directly or indirectly).
| id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
| name = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| circum = db.Column(db.Integer) |
There was a problem hiding this comment.
Keep in mind that the name we use here is what will literally be used for the db column name. So I'd suggest using the full circumference here.
| name = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| circum = db.Column(db.Integer) |
There was a problem hiding this comment.
Consider which columns we might want to make "required" (non-nullable). As it is, with this definition, we could make a Planet with a NULL name, description, and circumference. Would we want to allow this to happen?
| planets = Planet.query.all() | ||
| planets_response = [] | ||
| for planet in planets: | ||
| planets_response.append({"id": planet.id, "name": planet.name, "description": planet.description, "circumference in mkm": planet.circum}) |
There was a problem hiding this comment.
We strongly suggest making a helper method on the Planet class to take care of converting the model instance to a dictionary for the purpose of being returned.
There was a problem hiding this comment.
For the keys in our json dictionaries, prefer a name that can be a valid variable name (generally, prefer _ to spaces). While it doesn't impact our python handling that much, it can make the JSON handling for other languages (esp. JavaScript) if the keys can be treated as variables.
| @planets_bp.route("/<planet_name>", methods=["GET", "PUT", "DELETE"]) | ||
| def handle_one_planet(planet_name): |
There was a problem hiding this comment.
As with the previous endpoint, here too consider splitting the various verbs into separate functions.
| planet = Planet.query.get(planet_name) | ||
|
|
||
| if planet is None: | ||
| return make_response(f"Error: Planet {planet_name} not found", 404) |
There was a problem hiding this comment.
Be sure to jsonify any of the responses throughout.
| return make_response(f"Error: Planet {planet_name} not found", 404) | ||
|
|
||
| if request.method == "GET": | ||
| return {"id": planet.id, "name": planet.name, "description": planet.description, "circumference": planet.circum} |
There was a problem hiding this comment.
Consider moving the code to transform a Planet into a dictionary to a helper function in the Planet class.
| if request_body is None: | ||
| return make_response(f"Error: Request requires name, description, and circumference", 400) | ||
|
|
||
| elif "name" not in request_body or "description" not in request_body or "circum" not in request_body: | ||
| return make_response(f"Error: Request requires name, description, and circumference", 400) |
There was a problem hiding this comment.
Great! These are essentially the same as the checks we'd like to have for the POST method.
No description provided.