Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sea Turtles: Part 1 Solar System API: Hillary S. and Shannon B. #17

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
4 changes: 3 additions & 1 deletion app/__init__.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
from flask import Flask


def create_app(test_config=None):
app = Flask(__name__)

from .routes import planet_routes
app.register_blueprint(planet_routes.bp)
Comment on lines +26 to +27

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


return app
2 changes: 0 additions & 2 deletions app/routes.py

This file was deleted.

Empty file added app/routes/__init__.py
Empty file.
49 changes: 49 additions & 0 deletions app/routes/planet_routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
from flask import Blueprint, jsonify, abort, make_response

class Planet:
def __init__(self, id, name, description, has_moon=None):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.id = id
self.name = name
self.description = description
self.has_moon = has_moon

def make_dict(self):
return dict(
id = self.id,
name = self.name,
description = self.description,
has_moon = self.has_moon,
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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,  
        )


planets = [
Planet(1, "Mercury", "terrestrial", False),
Planet(2, "Jupiter", "gaseous", True),
Planet(3, "Earth", "terrestrial", True)
]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


bp = Blueprint("planets_bp",__name__, url_prefix="/planets")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


# GET /planets
@bp.route("", methods=["GET"])
def list_planets():

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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.

list_of_planets = [planet.make_dict() for planet in planets]

return jsonify(list_of_planets)

def validate_planet(id):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Validation method looks good, for both detecting a bad id, as well as an id with no record.

try:
id = int(id)
except ValueError:
abort(make_response(jsonify(dict(message=f"planet {id} is invalid")), 400))

for planet in planets:
if planet.id == id:
return planet

abort(make_response(jsonify(dict(message=f"planet {id} not found")), 404))

# GET planets/id
@bp.route("/<id>", methods=["GET"])
def get_planet(id):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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!

planet = validate_planet(id)
return jsonify(planet.make_dict())