Snow Leopards - Geiselle H. and Justina L. #21
Conversation
app/__init__.py
Outdated
| @@ -4,4 +4,7 @@ | |||
| def create_app(test_config=None): | |||
| app = Flask(__name__) | |||
|
|
|||
| from .routes import bp | |||
| app.register_blueprint(bp) | |||
app/routes.py
Outdated
| self.description = description | ||
| self.moons = moons | ||
|
|
||
| def to_planet_dict(self): |
There was a problem hiding this comment.
👀 The live code showed using a name like to_cat_dict, so I can see why you used to_planet_dict here. But it will be more useful going forward to all this simply to_dict going forward. If we have multiple models in our application, we like to have a method with a standardized name across all of them for converting them into a dictionary.
Consider the __str__ method that we overrode in Swap Meet. Each item subclass didn't have a differently name version of __str__ (e.g., __electronics_str__). They had a method with the same name so they could all be treated similarly.
So I'd suggest renaming this to to_dict and leave the type information to be implied by the instance we're calling this on. Most likely, the planet we're working with would be stored in a variable called planet. So with the current name, we end up writing planet.to_planet_dict(), which feels a little redundant. If we rename this to to_dict, it would become simply planet.to_dict().
app/routes.py
Outdated
| self.moons = moons | ||
|
|
||
| def to_planet_dict(self): | ||
| return dict( |
There was a problem hiding this comment.
👍 I like the dict() method for creating new dictionaries as well, since I can leave the quotes off the keys!
app/routes.py
Outdated
| Planet(3, "Earth", "where we live", 1) | ||
| ] | ||
|
|
||
| def validate_planet(planet_id): |
app/routes.py
Outdated
|
|
||
| abort(make_response({"message":f"planet {planet_id} not found"}, 404)) | ||
|
|
||
| bp = Blueprint("planets", __name__, url_prefix = "/planets") |
app/routes.py
Outdated
| results_list = [] | ||
|
|
||
| for planet in planets: | ||
| results_list.append(planet.to_planet_dict()) |
There was a problem hiding this comment.
👀 List comprehension opportunity here
app/routes.py
Outdated
| return jsonify(results_list) | ||
|
|
||
| @bp.route("/<planet_id>", methods=["GET"]) | ||
| def handle_one_planet(planet_id): |
There was a problem hiding this comment.
The Learn modules show naming the id matching part of the route with a model prefix. This can be helpful to distinguish among multiple ids in the route if we're working with nested routes.
When there's only a single id parameter in our route, we typically call it simply id as was done in the live code.
|
|
||
| from .routes import planet | ||
| from .routes import fictional_caretaker | ||
| app.register_blueprint(planet.bp) |
| @@ -0,0 +1,16 @@ | |||
| from app import db | |||
There was a problem hiding this comment.
Why "fictional" caretaker? You mean your planets don't have real caretakers? 🙂
Remember that we prefer to have a file containing a class definition to match the name of the class (allowing for differences in capitalization).
| db.session.add(new_caretaker) | ||
| db.session.commit() | ||
|
|
||
| return make_response(f"Caretaker {new_caretaker.name} successfully created", 201) |
There was a problem hiding this comment.
👀 Make sure to jsonify even plain string responses, otherwise flask will return an HTML response rather than JSON.
|
|
||
|
|
||
| @bp.route("/<caretaker_id>/planets", methods=["GET"]) | ||
| def read_one_planet(caretaker_id): |
There was a problem hiding this comment.
maybe call this get_caretaker_planets?
| @@ -0,0 +1,14 @@ | |||
| from flask import abort, make_response | |||
|
|
|||
| def validate_model(cls, model_id): | |||
There was a problem hiding this comment.
👍 Nice job on the reafactor, and moving it out of the original routes file.
|
|
||
| planet_query = Planet.query | ||
|
|
||
| if name_query: |
| planet.name = request_body["name"] | ||
| planet.description = request_body["description"] | ||
| planet.moons = request_body["moons"] |
There was a problem hiding this comment.
In the same way we made from_dict to centralize knowledge about the needed keys in the model itself, we could add a method like update_from_dict to apply updated values to an instance from a dictionary.
| @@ -0,0 +1,36 @@ | |||
| """added fictional caretaker | |||
There was a problem hiding this comment.
Nice to see that you went through a few migrations. Keep in mind that once you've gotten your "final" table layour squared away that it can be helpful to clear out all th old migrations, recreate the empty database, and get 1 nice, clean migration. Not something we'd want to do after having real data in a production database, but it's nice to start without migration clutter.
| return app.test_client() | ||
|
|
||
| @pytest.fixture | ||
| def one_saved_planet(app): |
There was a problem hiding this comment.
We would also want fixtures for the caretakers if there had been more time.
| response_body = response.get_data(as_text=True) | ||
| #Assert | ||
| assert response.status_code == 201 | ||
| assert response_body == "Planet Mars successfully created" |
There was a problem hiding this comment.
It would be a good idea to follow this up with a get call to confirm that the planet actually got stored in the database.
No description provided.