Phonix (Anees Quateja and Natalie sen) - #14
aneesquateja wants to merge 14 commits into
Conversation
apradoada
left a comment
apradoada
left a comment
There was a problem hiding this comment.
GREEN! Well done, y'all!
| from ..db import db | ||
| from app.models.planets import Planet | ||
|
|
||
| planets_bp = Blueprint("planets_bp", __name__, url_prefix="/planets") |
There was a problem hiding this comment.
Remember that we use our blueprints for every route that we write. While we don't have too many routes here, they can add up in more robust databases. It might not seem like much, but writing planets_bp over and over again can be quite cumbersome!
One way we get around this is with the naming convention of just bp. As long as we separate out our routes into different files, we can name all our blueprints bp. To keep our project from getting confused, we can go ahead an alias the different bps when we import them into our init.py. This would look like:
from .routes.planets_route import bp as planets_bp
| db.session.add(new_planet) | ||
| db.session.commit() | ||
|
|
||
| response = new_planet.to_dict() |
There was a problem hiding this comment.
Good use of the to_dict function here!
| description=self.description, | ||
| size=self.size | ||
| ) | ||
|
|
There was a problem hiding this comment.
In terms of refactoring, adding a from_dict class method could be super useful!
| name = request_body["name"] | ||
| description = request_body["description"] | ||
| size = request_body["size"] | ||
|
|
||
| new_planet = Planet(name=name, description=description, size=size) |
There was a problem hiding this comment.
If we had a from_dict method, we could condense all of this code into a single line!
This is also a great candidate for error handling in a refactoring scenario!
| query = db.select(Planet) | ||
| name_param = request.args.get("name") | ||
|
|
||
| if name_param: | ||
| # restrict to matching planet | ||
| query = query.where(Planet.name == name_param) | ||
|
|
||
| description_param = request.args.get("description") | ||
|
|
||
| if description_param: | ||
| # restrict to matching planet | ||
| query = query.where(Planet.description.ilike(f"%{description_param}%")) | ||
|
|
||
| size_param = request.args.get("size") | ||
| if size_param: | ||
| # restrict to matching planet | ||
| query = query.where(Planet.ilike(f"%{size_param}%")) | ||
|
|
||
| query = query.order_by(Planet.id) |
There was a problem hiding this comment.
As you start to add more models, you may end up seeing code like this repeated across models (similar to a validate_id). I wonder if there is a way you could extract this into a helper function that could work across models?
| def validate_planet(planet_id): | ||
| try: | ||
| planet_id = int(planet_id) | ||
| except: | ||
| abort(make_response({"message":f"Planet id {planet_id} invalid"}, 400)) | ||
|
|
||
| query = db.select(Planet).where(Planet.id == planet_id) | ||
| planet = db.session.scalar(query) | ||
|
|
||
| if not planet: | ||
| abort(make_response({"message": f"Planet id {id} not found"}, 404)) | ||
|
|
||
| return planet |
There was a problem hiding this comment.
This looks good! Don't forget that as we have multiple models, you can make this a more generic helper function that works across multiple classes!
| assert response.status_code == 200 | ||
| assert response_body == [] | ||
|
|
||
| def test_get_one_cat_succeeds(client, two_saved_planets): |
There was a problem hiding this comment.
Don't forget to change the test names when you're copying them over!
| response_body = response.get_json() | ||
|
|
||
| assert response.status_code == 201 | ||
| assert response_body == { |
There was a problem hiding this comment.
Here we know that the planet has been created, is there a way we can test to make sure it made it to the database?
| "id": 1 | ||
| } | ||
|
|
||
| def test_create_one_planet(client): |
There was a problem hiding this comment.
This test looks very similar to the previous test. It appears that the difference is adding a planet to an empty db vs a seeded one. Could you use the fixture you created to make these two tests different?
|
|
||
| } | ||
|
|
||
| def test_create_one_planet_with_planets_already_in_db(client, two_saved_planets): |
There was a problem hiding this comment.
You've answered my own question! While duplicate tests aren't an issue, it's best to try and test each case only once as there may be many cases to get through. With that in mind, I would go ahead and remove the previous test!
3 participants
No description provided.