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

Sharks - Jeannie Z. & Juli T. #15

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Sharks - Jeannie Z. & Juli T. #15

wants to merge 22 commits into from

Conversation

jterapin
Copy link

Completed Wave 1 & 2.

Copy link

@audreyandoy audreyandoy left a comment

Choose a reason for hiding this comment

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

Part 1 looks great! 👍

app/__init__.py Outdated
Comment on lines 6 to 7
from .routes import planet_bp
app.register_blueprint(planet_bp)

Choose a reason for hiding this comment

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

👍 Looks good! We can also move this file into a routes folder which would change the path of this import.

app/routes.py Outdated
Comment on lines 3 to 16
class Planet:
def __init__(self, id, name, description, distance_from_earth):
self.id = id
self.name = name
self.description = description
self.distance_from_earth = distance_from_earth

def to_json(self):
return {
"id": self.id,
"name": self.name,
"description": self.description,
"Distance from Earth": self.distance_from_earth
}

Choose a reason for hiding this comment

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

👍 Good work!

app/routes.py Outdated
planets = [
Planet(1, "Mars", "Next livable planet", "131.48 million mi"),
Planet(2, "Mercury", "Smallest planet", "94.025 million mi"),
Planet(3, "Earth", "We live here, slowly dying", "0.0 million mi")

Choose a reason for hiding this comment

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

Lol the description... 🤣 😭

app/routes.py Outdated
Comment on lines 27 to 37
def get_planets():
planets_response = []
for planet in planets:
planets_response.append(
{
"id": planet.id,
"name": planet.name,
"description": planet.description,
"Distance from Earth": planet.distance_from_earth
})
return jsonify(planets_response)

Choose a reason for hiding this comment

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

This is a good place to use the to_json() helper method from the Planet class:

def get_planets():
    planets_response = []
    for planet in planets:
        planets_response.append(planet.to_json())
    return jsonify(planets_response)

Choose a reason for hiding this comment

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

We can also use list comprehension to build planets_response.

app/routes.py Outdated
Comment on lines 39 to 48
def validate_planet(planet_id):
try:
planet_id = int(planet_id)
except:
return abort(make_response({"message": f"planet {planet_id} is invaild"}, 400))

for planet in planets:
if planet.id == planet_id:
return planet
return abort(make_response({"message": f"planet {planet_id} does not exist"}, 404))
Copy link

@audreyandoy audreyandoy Apr 28, 2022

Choose a reason for hiding this comment

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

The validation for detecting bad ids and ids with no record looks good!

app/routes.py Outdated
Comment on lines 51 to 54
@planet_bp.route("/<planet_id>", methods=["GET"])
def get_one_planet(planet_id):
planet = validate_planet(planet_id)
return jsonify(planet.to_json(), 200)

Choose a reason for hiding this comment

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

👍 nice use of the helper method!

Copy link

@audreyandoy audreyandoy left a comment

Choose a reason for hiding this comment

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

Jeannie and Julie! Great work in finishing solar system. I have added comments on small ways to improve the API.

🦈 ✨

Comment on lines +2 to +31
from flask_sqlalchemy import SQLAlchemy
from flask_migrate import Migrate
from dotenv import load_dotenv
import os


db = SQLAlchemy()
migrate = Migrate()
load_dotenv()


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


app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
if not test_config:
app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get(
"SQLALCHEMY_DATABASE_URI")
else:
app.config["TESTING"] = True
app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get(
"SQLALCHEMY_TEST_DATABASE_URI")

db.init_app(app)
migrate.init_app(app, db)
from app.models.planet import Planet

from .routes.planet_routes import planet_bp
app.register_blueprint(planet_bp)

Choose a reason for hiding this comment

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

👍 Good work in setting up the Flask app!

Comment on lines +6 to +8
name = db.Column(db.String)
description = db.Column(db.String)
color = db.Column(db.String)

Choose a reason for hiding this comment

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

Should we be able to make planets with no name? To prevent our API from creating a planet with a null name, we can set that column to nullable = False.

name = db.Column(db.String, nullable=False)

Comment on lines +10 to +30
def to_json(self):
return {
"id": self.id,
"name": self.name,
"description": self.description,
"color": self.color
}

def update(self, request_body):
self.name = request_body["name"]
self.description = request_body["description"]
self.color = request_body["color"]

@classmethod
def create(cls, request_body):
new_planet = cls(
name=request_body['name'],
description=request_body['description'],
color=request_body['color']
)
return new_planet

Choose a reason for hiding this comment

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

Nice use of helper methods and a class method!

Comment on lines +5 to +15
def validate_planet(planet_id):
try:
planet_id = int(planet_id)
except:
return abort(make_response(jsonify(f"Planet {planet_id} is invalid"), 400))

planet = Planet.query.get(planet_id)

if not planet:
return abort(make_response(jsonify(f"Planet {planet_id} does not exist"), 404))
return planet

Choose a reason for hiding this comment

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

Helpers look great! 👍 We may want to consider making the validation code as generic as possible to utilize any class and id or renaming this file to helper_planet_routes or planet_routes_helper. Either would be helpful if we expand this project to validate other objects like moons, suns, etc.

Choose a reason for hiding this comment

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

A more generic validation helper method would look like:

from flask import make_response, abort

def validate_object(cls, id):
    try:
        id = int(id)
    except:
        return abort(make_response({"message": f"{cls} {id} is invalid"}, 400))

    obj = cls.query.get(id)
    
    if not obj:
        abort(make_response({"message": f"{cls} {id} not found"},404))
    
    return obj

Comment on lines +8 to +17
@planet_bp.route("", methods=["POST"])
def create_planet():
request_body = request.get_json()

new_planet = Planet.create(request_body)

db.session.add(new_planet)
db.session.commit()

return make_response(jsonify(f"Planet {new_planet.name} has been created"), 201)

Choose a reason for hiding this comment

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

jsonify() is sufficient enough to generate a Response object

@planet_bp.route("", methods=["POST"])
def create_planet():
    request_body = request.get_json()

    new_planet = Planet.create(request_body)

    db.session.add(new_planet)
    db.session.commit()

    return jsonify(f"Planet {new_planet.name} has been created"), 201

Comment on lines +34 to +37
@ planet_bp.route("/<planet_id>", methods=["GET"])
def get_one_planet(planet_id):
planet = validate_planet(planet_id)
return make_response(jsonify(planet.to_json()), 200)

Choose a reason for hiding this comment

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

👍 Nice work!

Choose a reason for hiding this comment

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

Also, if a view function returns a dictionary (planet.to_json() in this case), Flask will automatically convert the dictionary into a JSON response object (aka Flask will turn dictionaries into JSON data for us). This means you can leave out make_response() like so:

@planets_bp.route("/<planet_id>", methods=["GET"])
def get_one_planet(planet_id):
    planet = validate_planet(planet_id)
    return planet.to_json(), 200

Here's the documentation with more info:
https://flask.palletsprojects.com/en/2.0.x/quickstart/#apis-with-json

Comment on lines +39 to +50
@ planet_bp.route("/<planet_id>", methods=["PUT"])
def update_planet(planet_id):
planet = validate_planet(planet_id)
request_body = request.get_json()

try:
Planet.update(request_body)
db.session.commit()
except KeyError:
return abort(make_response(jsonify("Missing information")), 400)

return make_response(jsonify(f"Planet #{planet.id} successfully updated"), 200)

Choose a reason for hiding this comment

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

Nice work! We may want to consider validating the request_body data in a separate helper method.

Comment on lines +52 to +59
@ planet_bp.route("/<planet_id>", methods=["DELETE"])
def delete_one_planet(planet_id):
planet = validate_planet(planet_id)

db.session.delete(planet)
db.session.commit()

return make_response(jsonify(f"Planet {planet.id} has been deleted"), 200)

Choose a reason for hiding this comment

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

👍 Comment about not needing make_response also applies.

Comment on lines +29 to +39
def two_saved_planets(app):
# Arrange
planet_one = Planet(name="Mars",
description="Close enough",
color="Red")
planet_two = Planet(name="Earth",
description="we out here",
color = "Green")

db.session.add_all([planet_one, planet_two])
db.session.commit()

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,73 @@
#`GET` `/planets` returns `200` and an empty array.

Choose a reason for hiding this comment

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

Test routes look great 👍 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants