C22 - Tatiana Snook (Sphinx) and Jen Nguyen (Phoenix)#4
C22 - Tatiana Snook (Sphinx) and Jen Nguyen (Phoenix)#4ItsJenOClock wants to merge 19 commits intoAda-C22:mainfrom
Conversation
refactoring
apradoada
left a comment
apradoada
left a comment
There was a problem hiding this comment.
Well done, y'all! I can tell y'all have a good grasp of building Flask APIs! Overall most of my comments are simply potential extension questions that you could think about, but no changes or updates are required!
| @@ -1,7 +1,21 @@ | |||
| from flask import Flask | |||
| from .routes.planet_routes import bp as planet_bp | |||
There was a problem hiding this comment.
Love that y'all aliased the bp as planet_bp!
| name=planet_data['name'], | ||
| description=planet_data['description'], | ||
| num_of_moons=planet_data['num_of_moons'] | ||
| ) |
There was a problem hiding this comment.
Great job on the to_dict and from_dict!
| if name_param: | ||
| query = query.where(Planet.name.ilike(f"%{name_param}%")) | ||
|
|
||
| description_param = request.args.get("description") | ||
| if description_param: | ||
| query = query.where(Planet.description.ilike(f"%{description_param}%")) | ||
|
|
||
| num_of_moons_param = request.args.get("num_of_moons") | ||
| if num_of_moons_param: | ||
| query = query.where(Planet.num_of_moons >= num_of_moons_param) | ||
|
|
||
| query = query.order_by(Planet.num_of_moons) |
There was a problem hiding this comment.
This isn't something you need to refactor, but I could definitely see the name_param and the description_param being similar across models. I wonder if there is a way these two queries could be extracted to a helper function in your route_utilities.py!
| query = query.order_by(Planet.num_of_moons) | ||
| planets = db.session.scalars(query) | ||
|
|
||
| planets_response = [planet.to_dict() for planet in planets] |
There was a problem hiding this comment.
Great use of a list comprehension!
| planet.name = request_body["name"] | ||
| planet.description = request_body["description"] | ||
| planet.num_of_moons = request_body["num_of_moons"] |
There was a problem hiding this comment.
One thing to think about here is the idea that we might not need to update all the attributes in Planet. Refactoring this wasn't a requirement, but one thing to think about as we continue to work with APIs is to how can we modify our update_planet in a way that allows us to only update certain parts of the planet?
|
|
||
| db.session.add_all([planet_1, planet_2]) | ||
|
|
||
| db.session.commit() |
There was a problem hiding this comment.
This conftest looks great!
| response_body = response.get_json() | ||
|
|
||
| assert response.status_code == 404 | ||
| assert response_body == {"message": f"Planet 1 not found"} |
There was a problem hiding this comment.
Small note here! The f-string you are using as the message is not necessary! It is true that the response we are returning from our route uses and f-string, but in the response itself, it it just a string. In fact, f-strings are just strings that we format differently. We would only need to use an f-string in this assert if we were bringing in a value. Because we aren't, you should be able to remove the f and it will still work!
| response_body = response.get_json() | ||
|
|
||
| assert response.status_code == 404 | ||
| assert response_body == {"message": f"Planet 3 not found"} |
There was a problem hiding this comment.
Same note as before with the f-string!
| "name": "Pluto", | ||
| "description": "dwarf planet", | ||
| "num_of_moons": 5 | ||
| } |
There was a problem hiding this comment.
These two create tests are great! One extension question to think about: How could we ensure the planets that have been created made it to the database? Hint: It might require a second API call!
| def test_delete_one_planet_with_planets_already_in_db(client, two_saved_planets): | ||
| response = client.delete("planets/1") | ||
| assert response.status_code == 204 | ||
| assert response.content_length is None |
There was a problem hiding this comment.
These look good! I'm not sure we necessarily need to assert that the content_length is None, but it can't hurt!
3 participants
No description provided.