Sea Turtles: Part 1 Solar System API: Hillary S. and Shannon B.#17
Sea Turtles: Part 1 Solar System API: Hillary S. and Shannon B.#17hillaryhsmith wants to merge 15 commits intoada-c17:mainfrom
Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good so far! Keep it up for Part 2, and into the project being introduced next week!
| from .routes import planet_routes | ||
| app.register_blueprint(planet_routes.bp) |
app/routes/planet_routes.py
Outdated
| from flask import Blueprint, jsonify, abort, make_response | ||
|
|
||
| class Planet: | ||
| def __init__(self, id, name, description, has_moon=None): |
There was a problem hiding this comment.
Since it appears that you expect has_moon to be a boolean value, the default valuewould be less surprising if it were either True or False (probably False). We used None with inventory in swap meet in order to act as sentinel value so we could tell whether the call had passed in a (mutable) inventory list. Here, we can use a boolean value directly as the default.
app/routes/planet_routes.py
Outdated
| def make_dict(self): | ||
| return dict( | ||
| id = self.id, | ||
| name = self.name, | ||
| description = self.description, | ||
| has_moon = self.has_moon, | ||
| ) |
There was a problem hiding this comment.
👍
PEP8 (python formatting standard) recommends for named arguments to not but a space around the = so that it looks less like we are make variables here. so
def make_dict(self):
return dict(
id=self.id,
name=self.name,
description=self.description,
has_moon=self.has_moon,
)| Planet(3, "Earth", "terrestrial", True) | ||
| ] | ||
|
|
||
| bp = Blueprint("planets_bp",__name__, url_prefix="/planets") |
|
|
||
| # GET /planets | ||
| @bp.route("", methods=["GET"]) | ||
| def list_planets(): |
There was a problem hiding this comment.
👍
Get all looks good. I like the function name you used too. Nice use of the list comprehension and instance method to build the dictionary for the instance.
app/routes/planet_routes.py
Outdated
|
|
||
| return jsonify(list_of_planets) | ||
|
|
||
| def validate_planet(id): |
There was a problem hiding this comment.
👍
Validation method looks good, for both detecting a bad id, as well as an id with no record.
app/routes/planet_routes.py
Outdated
|
|
||
| # GET planets/id | ||
| @bp.route("/<id>", methods=["GET"]) | ||
| def get_planet(id): |
There was a problem hiding this comment.
👍
Single planet endpoint looks good. We're able to reuse that dictionary instance method and move most of the logic into the validation method. We could get really crazy and even write the entire body as
return jsonify(validate_planet(id).make_dict()) Though it's probably clearer the way you wrote it!
app/routes/planet_routes.py
Outdated
| planets = [ | ||
| Planet(1, "Mercury", "terrestrial", False), | ||
| Planet(2, "Jupiter", "gaseous", True), | ||
| Planet(3, "Earth", "terrestrial", True) | ||
| ] |
There was a problem hiding this comment.
We'll be getting rid of this collection soon, but consider using named arguments here to help this be a little more self-documenting about what the various values are. Id and name are pretty clear, but it might be harder to tell that the second string is a description, and I'd be hard pressed to guess that the final boolean was about whether there was a moon there.
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Nice job finishing up solar system. This is a great foundation to keep working from for Task List and more! I included an idea for how to do the multi-parameter filtering you asked about. Keep doing your own research from Flask/SQLAlchemy tutorials, etc for additional thoughts and approaches. Also, keep an eye out for additional refactoring and error handling as you continue to work with APIs!
| app = Flask(__name__) | ||
|
|
||
| if not test_config: | ||
| app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False |
There was a problem hiding this comment.
Notice that since this option is being set in both branches of the if, it could be pulled out of the branches to either before or after the if to remove the repetition.
| self.has_moon = data_dict["has_moon"] | ||
|
|
||
| def replace_some_details(self, data_dict): | ||
| planet_keys = data_dict.keys() |
There was a problem hiding this comment.
We don't need to retrieve the keys separately here. The in operator on dictionaries checks for a key itself, so the later checks could be written as:
if "name" in data_dict:
# handle name key...| has_moon=self.has_moon, | ||
| ) | ||
|
|
||
| def replace_all_details(self, data_dict): |
| self.description = data_dict["description"] | ||
| self.has_moon = data_dict["has_moon"] | ||
|
|
||
| def replace_some_details(self, data_dict): |
|
|
||
| # ************************* | ||
| @classmethod | ||
| def from_dict(cls, data_dict): |
| if description_param: | ||
| planets = Planet.query.filter_by(description=description_param) | ||
| elif name_param: | ||
| planets = Planet.query.filter_by(name=name_param) | ||
| elif has_moon_param: | ||
| planets = Planet.query.filter_by(has_moon=has_moon_param) | ||
| else: | ||
| planets = Planet.query.all() |
There was a problem hiding this comment.
Hillary, you wrote that you wanted to write this in such a way that we could filter based on some combination of parameters. To do so, we can take advantage of the fact that until we call all() or try to iterate over the results, all of these methods return objects that represent the configured query, not the result. In fact, lots of the query methods in SQLAlchemy work this way (though it's hard to tell from the docs), allowing us to chain together complex queries.
As an example of this, consider the approach below:
planets = Planet.query
if description_param:
planets = planets.filter_by(description=description_param)
if name_param:
planets = planets.filter_by(name=name_param)
if has_moon_param:
planets = planets.filter_by(has_moon=has_moon_param)
planets = planets.all()If no filters are provided, the final line effectively becomes Planet.query.all(), but if filters are present, the chain on additional filter_by calls as needed.
This strategy could even be used to add on paging or ordering behavior!
|
|
||
| # GET /planets/<planet_id> | ||
| @bp.route("/<planet_id>", methods=["GET"]) | ||
| def get_planet_by_id(planet_id): |
|
|
||
| # PUT /planets/<planet_id> | ||
| @bp.route("/<planet_id>", methods=["PUT"]) | ||
| def replace_planet_by_id(planet_id): |
| db.session.delete(planet) | ||
| db.session.commit() | ||
|
|
||
| return make_response(f"Planet with id {planet_id} successfully deleted") |
There was a problem hiding this comment.
Be sure to jsonify this error message. If we don't, Flask will assume we meant this to be text/html Content-Type rather than application/json. This can cause problems for clients when trying to retrieve the contents of the response.
|
|
||
| # PATCH /planets/<planet_id> | ||
| @bp.route("/<planet_id>", methods=["PATCH"]) | ||
| def update_planet_by_id(planet_id): |
5 participants
No description provided.