Sea Turtles - Georgia A & Lili P by georgia-adam · Pull Request #9 · ada-c17/solar-system-api

georgia-adam · 2022-04-26T19:36:46Z

No description provided.

…ion, make a Planet instance method to_dict

anselrognlie

Looks good so far! I'd just suggest a little reorganization of the route file to its own folder, and keep pushing yourselves to try python syntax like list comprehensions when you have the chance.

anselrognlie · 2022-04-27T03:54:36Z

@@ -1,2 +1,53 @@
-from flask import Blueprint
+from flask import Blueprint, jsonify, abort, make_response


Consider moving this file into a routes folder. This is useful even when we don't have another set of routes to store there, since we'll be adding additional files, and having things grouped logically can help us find our way around our code.

anselrognlie · 2022-04-27T03:55:03Z

+    from .routes import bp
+    app.register_blueprint(bp)


👍

If you move the routes.py file this will need to change a little.

anselrognlie · 2022-04-27T03:57:16Z

+from flask import Blueprint, jsonify, abort, make_response

+class Planet:
+    def __init__(self, id, name, description, life):


👍

Consider renaming life to has_life. This helps us know to expect a boolean value. Boolean variables/attributes are often named as has_something or is_something. Essentially, we try to phrase the name so that it implies that this is a yes/no, or true/false value.

anselrognlie · 2022-04-27T03:57:48Z

+        self.description = description
+        self.life = life
+
+    def to_dict(self):


👍

Instance method for building a dictionary from an instance looks good! (Again, consider renaming life.)

anselrognlie · 2022-04-27T03:59:55Z

+planets = [
+    Planet(1, "Earth", "Best planet", True),
+    Planet(2, "Saturn", "Got a ring on it", False),
+    Planet(3, "Mars", "We want to go there", False)
+]


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 life there.

anselrognlie · 2022-04-27T04:00:07Z

+    Planet(3, "Mars", "We want to go there", False)
+]
+
+bp = Blueprint("planets_bp", __name__, url_prefix="/planets")


anselrognlie · 2022-04-27T04:01:48Z

+bp = Blueprint("planets_bp", __name__, url_prefix="/planets")
+
+
+def validate_planet(id):


Nice decision to pull this up above the two routes. And the validation method looks good, for both detecting a bad id, as well as an id with no record.

anselrognlie · 2022-04-27T04:02:46Z

+
+
+@bp.route("", methods=["GET"])
+def get_planets():


👍

Consider using a list comprehension for generating the result list. This would be especially nice since you have the to_dict instance method.

anselrognlie · 2022-04-27T04:03:31Z

+    return jsonify(planets_list)
+
+@bp.route("/<id>", methods=["GET"])
+def get_one_planet(id):


👍

Single planet endpoint looks good. We're able to reuse that dictionary instance method and move most of the logic into the validation method.

…refactored GET endpoint for Planets, and some other small refactoring

…eated error_message function

anselrognlie

Nice job finishing up solar system. This is a great foundation to keep working from for Task List and more! Keep an eye out for additional refactoring and error handling as you continue to work with APIs.

anselrognlie · 2022-05-09T15:27:22Z

@@ -0,0 +1,68 @@
+from unicodedata import name


Looks like a stray import from VS Code got in here. Thanks, VS Code!

Also, since you moved this file into a routes folder, consider naming this file something like planet_routes.py. This is helpful when we have routes for multiple model types, and also makes the import in the app init a little more legible.

Don't forget to add a __init__.py file in this directory. Without it, python is actually doing a slightly older style of import that can lead to trouble down the road.

anselrognlie · 2022-05-09T15:28:19Z

+from .routes_helper import validate_planet, error_message
+
+
+planets_bp = Blueprint("planets", __name__, url_prefix="/planets")


When a route file has only a single blueprint in it (pretty typical), consider naming the variable simply bp

anselrognlie · 2022-05-09T15:32:47Z

+    from app.routes.routes import planets_bp
+    app.register_blueprint(planets_bp)


If you renamed things according to my later suggestions, these lines could be re-written as:

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

anselrognlie · 2022-05-09T15:33:50Z

+migrate = Migrate()
+load_dotenv()

 def create_app(test_config=None):


anselrognlie · 2022-05-09T15:34:38Z

@@ -0,0 +1,34 @@
+"""adds Planet model


👍 Nice migration message

anselrognlie · 2022-05-09T16:27:26Z

+
+
+@planets_bp.route("", methods=["GET"])
+def get_planets():


anselrognlie · 2022-05-09T16:28:15Z

+
+
+@planets_bp.route("/<planet_id>", methods=["GET"])
+def get_planet_by_id(planet_id):


anselrognlie · 2022-05-09T16:33:06Z

+        planet.name = request_body["name"]
+        planet.description = request_body["description"]
+        planet.has_life = request_body["has_life"]


Consider moving this to an instance method in Planet.

anselrognlie · 2022-05-09T16:35:33Z

+        error_message(f"Missing key {err}", 400)
+
+    db.session.commit()
+    return make_response(f"Planet {planet_id} successfully updated!", 200)


Make sure to jsonify this string, otherwise Flask won't return it as a JSON string. It will be returned as HTML. And here, if we jsonify we don't need the make_response. Also, keep in mind that 200 is the default response code, so we could leave that off.

return jsonify(f"Planet {planet_id} successfully updated!"), 200

anselrognlie · 2022-05-09T16:37:59Z

+
+    db.session.delete(planet)
+    db.session.commit()
+    return make_response(f"Planet {planet_id} successfully deleted.")


Here, too, make sure to jsonify this.

lili4x4 and others added 2 commits April 22, 2022 14:02

Added Planets class and planets endpoint

b7ceb35

creating get_one_planet endpoint, make a validate planet helper funct…

bb3c58a

…ion, make a Planet instance method to_dict

anselrognlie reviewed Apr 27, 2022

View reviewed changes

lili4x4 and others added 8 commits April 29, 2022 15:15

Refactored and made Planet model, created POST endpoint for Planets, …

c0d5705

…refactored GET endpoint for Planets, and some other small refactoring

added put and delete

29710d3

delete added

691f864

Moved validate_planet function

982ece0

Added query param 'name' to get_planets, added routes_helper file, cr…

cef35b2

…eated error_message function

wave 6 draft implementations

189cfea

tests working

ce5c4ef

all waves complete

224dcac

anselrognlie reviewed May 9, 2022

View reviewed changes

		@@ -1,2 +1,53 @@
		from flask import Blueprint
		from flask import Blueprint, jsonify, abort, make_response

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


		def validate_planet(id):

		from .routes_helper import validate_planet, error_message


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

		from app.routes.routes import planets_bp
		app.register_blueprint(planets_bp)



		@planets_bp.route("/<planet_id>", methods=["GET"])
		def get_planet_by_id(planet_id):

Conversation

georgia-adam commented Apr 26, 2022

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anselrognlie Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

anselrognlie Apr 27, 2022 •

edited

Loading