Sea Turtles - Shayla and Tamara #10

Myranae · 2022-04-26T20:06:53Z

No description provided.

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-27T04:21:56Z

app/routes.py

@@ -1,2 +1,55 @@
-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-27T04:22:25Z

app/__init__.py

+    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-27T04:24:01Z

app/routes.py

+    def to_dict(self):
+        return dict(
+        id=self.id,
+        name=self.name,
+        description=self.description,
+        color=self.color,
+        )


👍

This function to build a dictionary looks good. Formatting-wise, generally I indent the wrapped contents of a function call like this.

def to_dict(self): return dict( id=self.id, name=self.name, description=self.description, color=self.color, )

anselrognlie · 2022-04-27T04:26:27Z

app/routes.py

+planets = [
+    Planet(1, "Mercury", "Super small, comparatively", "slate gray"),
+    Planet(2, "Venus", "It's sooo hot!", "yellow-white"),
+    Planet(3, "Earth", "Home to humans", "blue-green"),
+    Planet(4, "Mars", "The red one", "red"),
+    Planet(5, "Jupiter", "The biggest of them all", "orange-yellow"),
+    Planet(6, "Saturn", "What beautiful rings it has!", "hazy yellow-brown"),
+    Planet(7, "Uranus", "Tilted sideways", "blue-green"),
+    Planet(8, "Neptune", "Giant, stormy, blue", "blue"),
+    Planet(9, "Maybe Pluto", "Is it really a planet??", "reddish-brown")
+    ]


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, and description and color are probably ok, but having those named arguments would make it explicit, especially in a situation like this where the arguments are mostly a bunch of the same data type.

anselrognlie · 2022-04-27T04:26:35Z

app/routes.py

+    Planet(9, "Maybe Pluto", "Is it really a planet??", "reddish-brown")
+    ]
+
+bp = Blueprint("planets", __name__, url_prefix="/planets")


anselrognlie · 2022-04-27T04:27:30Z

app/routes.py

+
+bp = Blueprint("planets", __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:28:28Z

app/routes.py

+    abort(make_response({"Message":f"Planet ID {id} not found"}, 404))
+
+@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:29:16Z

app/routes.py

+    return jsonify(results_list)
+
+@bp.route("/<id>", methods=["GET"])
+def get_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. Note that the default status code is 200, so you could leave that off the return.

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-09T21:34:19Z

app/__init__.py


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

 def create_app(test_config=None):


anselrognlie · 2022-05-09T21:35:40Z

app/models/planet.py

+    name = db.Column(db.String)
+    description = db.Column(db.String)
+    color = db.Column(db.String)


Consider setting some of these columns to have nullable=false. For example, should we be able to create a planet without a name?

anselrognlie · 2022-05-09T21:36:26Z

migrations/versions/fdb0b190d588_adds_planet_model.py

@@ -0,0 +1,34 @@
+"""Adds planet model


👍 Good descriptive message

anselrognlie · 2022-05-09T21:38:41Z

tests/conftest.py

+@pytest.fixture
+def create_two_planets(app):
+    planet_one = Planet(
+        name="Mercury",
+        description="smallest planet",
+        color="magenta"
+    )
+    planet_two = Planet(
+        name="Mars",
+        description="maybe we will live here?",
+        color="reddish brown"
+    )
+    db.session.add_all([planet_one, planet_two])
+    db.session.commit()


👍
Because our SQL table is set to assign ids, it's fine to leave off the ids for the records as you did here.

The migration logic sets up the id behavior a little differently from how we did it when we worked with SQL manually, and would also allow us to set the ids explicitly. One nice thing about setting the id explicitly in a fixture is that we know what id to use in subsequent tests. If we don't, then we should still be able to assume that the ids will start from 1 and increment, but we probably shouldn't depend on it. So if we leave off the ids, then we'd probably want to return the two records in a list so that the test using these records could get their ids safely, regardless of what they were set to. This would look like:

@pytest.fixture def create_two_planets(app): planet_one = Planet( name="Mercury", description="smallest planet", color="magenta" ) planet_two = Planet( name="Mars", description="maybe we will live here?", color="reddish brown" ) db.session.add_all([planet_one, planet_two]) db.session.commit() return [planet_one, planet_two]

And then in a test using the create_two_planets fixture, we could get the id of the first planet as create_two_planets[0].id

anselrognlie · 2022-05-09T21:42:45Z

tests/test_routes.py

+
+    #assert
+    assert response.status_code == 404
+    assert response_body == None


Notice that in your 404 handling code, you return a message from the route, but the response body ends up being None. This is because the 404 handling code is using an un-jsonify'd string in the response, which Flask interprets as HTML rather than JSON. And if the response isn't set as JSON, then the call to get_json() returns None.

anselrognlie · 2022-05-09T21:58:03Z

app/routes/planet_routes.py

+    new_planet = Planet(
+        name = request_body["name"],
+        description = request_body["description"],
+        color = request_body["color"]
+    )


This would be great to move into a @classmethod in the Planet model class.

And consider what would happen if one of the keys were missing from the request.

anselrognlie · 2022-05-09T21:59:08Z

app/routes/planet_routes.py

+    db.session.add(new_planet)
+    db.session.commit()
+
+    return make_response(jsonify(f"Planet {new_planet.name} successfully created"), 201)


I mentioned this in the test file as well, but consider returning the jsonify'd planet here rather than a status message, so the caller knows the id of the resource that was just created.

anselrognlie · 2022-05-09T22:01:20Z

app/routes/planet_routes.py

+    planets = Planet.query
+
+    if name_param:
+        planets = planets.filter_by(name=name_param)
+    if description_param:
+        planets = planets.filter_by(description=description_param)
+    if color_param:
+        planets = planets.filter_by(color=color_param)
+
+    planets = planets.all()


Love the stacked query handling. 🙂

Check out https://docs.sqlalchemy.org/en/13/orm/tutorial.html#common-filter-operators for some other examples of the kinds of queries we can build up.

anselrognlie · 2022-05-09T22:02:41Z

app/routes/planet_routes.py

+        planets_response.append(
+            {
+                "id": planet.id,
+                "name": planet.name,
+                "description": planet.description,
+                "color": planet.color
+            }
+        )


This would be a greta spot to use your to_dict helper.

planets_response.append(planet.to_dict())

And we might consider using a list comprehension since it's so short now:

planets_response = [planet.to_dict() for planet in planets]

anselrognlie · 2022-05-09T22:10:34Z

app/routes/planet_routes.py

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


Consider moving this to an instance method in Planet. And again, consider what would happen if one of the keys were missing.

Myranae and others added 3 commits April 22, 2022 14:02

passes wave 1 part 1 requirements

823ef3c

partial progress on wave 2

f40beb4

Added functionality to read one planet and provide custom error messages

510d039

anselrognlie reviewed Apr 27, 2022

View reviewed changes

Myranae and others added 17 commits April 29, 2022 14:02

sets up database, table, and planet model

d5b47ef

added POST route

560597a

sets up the read all planets route

5e3f63c

adds read_one_planet route and to_dict method

496c2d4

added validate_planet and delete planet

9aedb80

adds update route

8337383

refactors route file to new folder

875547d

adds in filtering all planets by query params

4b8a05d

Added automated test setup

9c6eb19

added conftest

b47b247

creates dummy test data and three tests

130ae26

added test for post

ebb7fa1

added jsonify to response

a7fc725

uncomments a test

dcd5386

added test for posts

ec693fc

Merge branch 'main' of https://github.com/Myranae/solar-system-api

1425728

added tests for put and delete

47a746d

anselrognlie reviewed May 9, 2022

View reviewed changes

added Procfile

47a7311

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sea Turtles - Shayla and Tamara #10

Sea Turtles - Shayla and Tamara #10

Myranae commented Apr 26, 2022

anselrognlie left a comment

anselrognlie Apr 27, 2022

anselrognlie Apr 27, 2022

anselrognlie Apr 27, 2022

anselrognlie Apr 27, 2022

anselrognlie Apr 27, 2022

anselrognlie Apr 27, 2022

anselrognlie Apr 27, 2022

anselrognlie Apr 27, 2022

anselrognlie left a comment

anselrognlie May 9, 2022

anselrognlie May 9, 2022

anselrognlie May 9, 2022

anselrognlie May 9, 2022

anselrognlie May 9, 2022

anselrognlie May 9, 2022

anselrognlie May 9, 2022

anselrognlie May 9, 2022 •

edited

Loading

anselrognlie May 9, 2022

anselrognlie May 9, 2022

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


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

		def validate_planet(id):

Sea Turtles - Shayla and Tamara #10

Are you sure you want to change the base?

Sea Turtles - Shayla and Tamara #10

Conversation

Myranae commented Apr 26, 2022

anselrognlie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anselrognlie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anselrognlie May 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anselrognlie May 9, 2022 •

edited

Loading