Conversation
| all_planets_response = [] | ||
| for planet in planets: | ||
| all_planets_response.append(planet.to_dict()) |
There was a problem hiding this comment.
This would be a good candidate for list comprehension, something like:
all_planets_response = [planet.to_dict() for planet in planets]
|
@jdanielle12 @lulusde1210 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! |
jdanielle12
changed the title
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Nice work on Part 2 of solar-system. Left a couple of minor comments. Hope they help with task-list. Let me know if you have any questions!
|
|
||
| def create_app(test_config=None): | ||
| app = Flask(__name__) | ||
| app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False |
| @@ -0,0 +1,27 @@ | |||
| from app import db | |||
| except KeyError as e: | ||
| message = jsonify(f"missing required value: {e}") | ||
| abort(make_response(message, 400)) |
There was a problem hiding this comment.
Nice job using try/except to add in error handling for a request that relies on (possibly incorrect) client data.
| if name_query: | ||
| planets = Planet.query.filter_by(name=name_query) | ||
| else: | ||
| planets = Planet.query.all() | ||
|
|
||
| results = [planet.to_dict() for planet in planets] |
| planet_to_update = validate_model(Planet, id) | ||
|
|
||
| planet_to_update.name = planet_data["name"] | ||
| planet_to_update.description = planet_data["description"] | ||
| planet_to_update.distance_from_sun = planet_data["distance_from_sun"] | ||
| db.session.commit() | ||
|
|
||
| message = jsonify(f"Planet {planet_to_update.name} updated") | ||
| return make_response(message, 200) |
There was a problem hiding this comment.
Just like your POST request had error handling, your PUT request also needs it too. If a client sends a PUT req, but the request body had "planet_name" instead of "name", line 75 will throw an error (500 status code), but what we want our route to do is respond with an error message that gives the client feedback about what went wrong.
| ) | ||
| db.session.add(Jupiter) | ||
| db.session.add(Mercury) | ||
| db.session.commit() |
There was a problem hiding this comment.
Something you could do here is to create a list and add the 2 planets to it, then return the list. That way when a unit test brings in this fixture two_planets you actually get the list of 2 saved planets. Right now this fixture doesn't return anything.
So that would look like
list_of_planets = [jupiter, mercury]
| Jupiter = Planet( | ||
| name="Jupiter", | ||
| description="Jupiter has sixty-seven moons", | ||
| distance_from_sun=48.4 | ||
| ) | ||
| Mercury = Planet( |
There was a problem hiding this comment.
variable names should begin with lowercase letters
| assert response_body == [{ | ||
| "id": 1, | ||
| "name": "Jupiter", | ||
| "description": "Jupiter has sixty-seven moons", | ||
| "distance_from_sun": "48.4" | ||
| }, | ||
| { | ||
| "id": 2, | ||
| "name": "Mercury", | ||
| "description": "Mercury has zero moons", | ||
| "distance_from_sun": "3.5" | ||
| }] |
There was a problem hiding this comment.
Per my comment above in the two_planets fixture, if that fixture returned a list of your 2 saved planets then you could check the response_body against your fixture (where you saved the planets when this test first runs)!
saved_planet_1 = two_planetes[0]
assert response_body["id"] == saved_planet_1.id
assert response_body["name"] == saved_planet_1.name
assert response_body["description"] == saved_planet_1.description
assert response_body["distance_from_sun"] == saved_planet_1.distance_from_sunThis would make the test more robust
| response = client.get("/planets/1") | ||
| response_body = response.get_json() | ||
|
|
||
| assert response.status_code == 200 | ||
| assert response_body["id"] == 1 | ||
| assert response_body["name"] == "Jupiter" | ||
| assert response_body["description"] == "Jupiter has sixty-seven moons" | ||
| assert response_body["distance_from_sun"] == "48.4" |
There was a problem hiding this comment.
Would be good to compare the response_body with what was saved in the database when the fixture ran instead of comparing the response_body to string literals.
If you refactored two_planets to return a list of saved planet instances, then you can refactor the test. See my comment in test_all_planets_return_a_list_of_planets as an example.
| assert response.status_code == 201 | ||
| assert response_body == f"Planet {EXPECTED_PLANET['name']} successfully created" | ||
| assert actual_planet.name == EXPECTED_PLANET["name"] | ||
| assert actual_planet.description == EXPECTED_PLANET["description"] | ||
| assert actual_planet.distance_from_sun == EXPECTED_PLANET["distance_from_sun"] |
2 participants
No description provided.