Conversation
| for planet in planets: | ||
| result.append(planet.make_planet_dict()) |
There was a problem hiding this comment.
This would be a good candidate for list comprehension, something like:
results = [planet.make_planet_dict() for planet in planets]
| @planet_bp.route("/<id>", methods=["GET"]) | ||
| def get_planet(id): | ||
| planet = validate_planet(id) | ||
| return planet.make_planet_dict() |
There was a problem hiding this comment.
It's not a rule and the code still executes just fine, but usually we'd see all the routes grouped together and all the helpers methods grouped together in a file.
|
@johanna-j-c @yvett-codes Great work on waves 1 and 2 for Solar System, team! It looks like you've got a handle on creating a blueprint, registering it with your Flask app and writing routes. Nice job refactoring your code so that the routes can stay short and sweet! |
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Nice work on Part 2 of Solar System! I left a couple of small comments. Y'all seem to be in good shape wrt writing backend code using Flask and SQLAlchemy 🥳
| app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get("SQLALCHEMY_DATABASE_URI") | ||
| else: | ||
| app.config["TESTING"] = True | ||
| app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False |
There was a problem hiding this comment.
Notice that line 15 and 19 are the same. Since this line of code needs to execute for both conditions, you can bring it outside the if/else block.
| except KeyError as e: | ||
| abort(make_response({"message": f"Missing required value {e}"}, 400)) |
There was a problem hiding this comment.
Nice try/except to handle missing data when creating a Planet.
|
|
||
| @planet_bp.route("/<id>", methods=["GET"]) | ||
| def get_planet(id): | ||
| planet = validate_model(Planet, id) |
There was a problem hiding this comment.
Nice work using the updated validate_model method and passing in the class of the object we want to validate
|
|
||
| return jsonify(results) | ||
|
|
||
| def validate_model(cls, model_id): |
| planet_to_update.name = planet_data["name"] | ||
| planet_to_update.description = planet_data["description"] | ||
| planet_to_update.radius = planet_data["radius"] |
There was a problem hiding this comment.
Just like you had error handling in your POST request, you want to add try/except to handle missing data when updating a Planet. Right now, if the request body sent with this PUT request didn't have a key "description", then line 65 would throw an exception that doesn't get handled. We should anticipate that a request body might have missing or bad data and try to handle it our route.
|
|
||
| db.session.add_all(planets) | ||
| db.session.commit() | ||
| return planets No newline at end of file |
There was a problem hiding this comment.
Nice work writing up the boiler plate code needed for the app and client fixtures and also writing a couple of custom fixtures for saving some planets to the db!
| response_body = response.get_json() | ||
|
|
||
| assert response.status_code == 201 | ||
| assert response_body == "Planet Yoshi successfully created." |
There was a problem hiding this comment.
A way to make this test more robust would to check the response_body against the request body that you create for the post request.
The dictionary literal you create on lines 42-26 should be pulled into a variable like:
EXPECTED_PLANET = {
"name": "Yoshi",
"description": "Adorable and egg filled planet.",
"radius": "7,156.9 mi"
}
response = client.post("/planets", json=EXPECTED_PLANET)Then you could write some assertions like:
actual_planet = Planet.query.get(1) # since we create new table at the start of each test, we can assume that when your test makes a POST request to the DB that there will only be one planet in there so we can query for a record with id == 1
assert actual_planet.name == EXPECTED_PLANET["name"]
assert actual_planet.description == EXPECTED_PLANET["description"]
assert actual_planet.radius == EXPECTED_PLANET["radius"]|
|
||
| assert response.status_code == 404 | ||
|
|
||
| def test_get_all_planets_with_valid_data(client, multiple_planets): |
There was a problem hiding this comment.
This is a thorough test! Nice work 👍
3 participants
Part 1