Zoisite- Katherine G and Madi J #20
Conversation
app/routes.py
Outdated
| planets_response = [] | ||
| for planet in planets: | ||
| planets_response.append(planet.to_dict()) |
There was a problem hiding this comment.
This would be a good candidate for a list comprehension, what could that look like?
|
|
||
| else: | ||
| app.config["TESTING"] = True | ||
| app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False |
There was a problem hiding this comment.
The line app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False is duplicated in both parts of the if/else. We could move that line to either above or below the if/else, so it only needs to be written once.
| from app.models.planet import Planet | ||
| from flask import Blueprint, jsonify, abort, make_response, request | ||
|
|
||
| def validate_model(cls,model_id): |
There was a problem hiding this comment.
Tiny style best practices note: we should add a space between each parameter to a function to give readers a visual separation, this helps people read and parse lines easier & faster.
| try: | ||
| model_id = int(model_id) | ||
| except: | ||
| abort(make_response({"error message": f"{cls.__name__} {model_id} is invalid"}, 400)) |
There was a problem hiding this comment.
These lines are a little long for PEP8 best practices, how could we rearrange things over a couple lines to stay under 79 characters?
| @@ -0,0 +1,16 @@ | |||
| from app import db | |||
There was a problem hiding this comment.
Nice use of helper.py for organization ^_^
|
|
||
| class Planet(db.Model): | ||
| id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
| name = db.Column(db.String, nullable=False) |
There was a problem hiding this comment.
Nice use of nullable, it'd be hard to know what planet we're working with if it doesn't have a name!
app/routes.py
Outdated
| planet.name = request_body["name"] | ||
| planet.description = request_body["description"] | ||
| planet.num_moons = request_body["num_moons"] |
There was a problem hiding this comment.
What happens if the user forgot a key? What error handling could be helpful for the user?
app/routes.py
Outdated
|
|
||
| db.session.commit() | ||
|
|
||
| return make_response(f"Planet {planet.id} successfully updated") |
There was a problem hiding this comment.
If we want to be sure we're consistent about the format we're using to send back messages, we could wrap the message in a call to jsonify like we do in create_planets to ensure the response is in JSON format.
|
|
||
|
|
There was a problem hiding this comment.
Tiny style nitpick: our whitespace should be meaningful inside a file. Is there a distinction we're trying to draw between the app fixture and other fixtures in this file? If not, I suggest using a single blank line to separate the functions for consistency.
| ) | ||
| db.session.add(planet) | ||
| db.session.commit() | ||
| return planet |
There was a problem hiding this comment.
Neat choice to return the planet so you can use its values for the assertions in the test test_get_one_planet_returns_seeded_planet.
| assert planet_one.name == EXPECTED_PLANET_ONE_NAME["name"] | ||
| assert planet_two.description == EXPECTED_PLANET_TWO_DESCRIPTION["description"] | ||
| assert planet_three.num_moons == EXPECTED_PLANET_THREE_NUM_MOONS["num_moons"] |
There was a problem hiding this comment.
We've confirmed a single attribute's value on each model, but this doesn't guarantee the data is accurate on all the models. It's better practice to check all the attributes on the relevant models to ensure there are no strange issues. (A hypothetical situation: what if the name field wasn't being saved after the first model was created? By only checking the name on the first Planet, we wouldn't catch this.)
No description provided.