wave 1 and wave 2 by Danqing and Celina Group #4 by DQLIU1995 · Pull Request #21 · Ada-C19/solar-system-api

DQLIU1995 · 2023-04-25T20:49:03Z

No description provided.

anselrognlie

You're off to a great start. Keep it up for the following waves! 🌊

Be sure to add an additional attribute to your Planet class.

anselrognlie · 2023-04-28T01:30:40Z

    app = Flask(__name__)

-    return app
+    from .routes import bp


anselrognlie · 2023-04-28T01:38:03Z

+from flask import Blueprint, jsonify, abort, make_response
+
+class Planet:
+    def __init__(self, id, name, description):


👀 Note that the project asked to

Define a Planet class with the attributes id, name, and description, and one additional attribute

Please add an additional attribute of your choosing to the Planet. This could any additional piece of information you'd like to track about the planet. Maybe some zodiacal association, discovery date, mass... really anything at all.

anselrognlie · 2023-04-28T01:39:46Z

+        self.name = name
+        self.description = description 
+
+    def make_planet_dict(self):


✅ Nice helper to build a dict representing this type. The name here follows from the live code example, but prefer not mentioning the type in the function name. The type is implied by class we're in and doesn't need to be mentioned in the function name. So here something like make_dict would be more expected.

Consider that we would probably have a planet instance in a variable named something like planet. As written, calling the code to make a dict would look like

planet_dict = planet.make_planet_dict()

which is a little redundant. If we shorten the name, it would be

planet_dict = planet.make_dict()

We can still see that we're making a dict from a planet instance.

anselrognlie · 2023-04-28T01:42:13Z

+planets = [
+    Planet(1, "Apple Planet", "A planet that only grows Apple"),
+    Planet(2, "Orange Planet", "A planet that only grows orange"),
+    Planet(3, "Pear Planet", "A planet that only grows pear")


Stylistically, consider leaving a trailing , after the final item, which makes it easier to add additional items later if needed.

anselrognlie · 2023-04-28T01:43:44Z

+    Planet(3, "Pear Planet", "A planet that only grows pear")
+]
+
+bp = Blueprint("planets", __name__, url_prefix="/planets")


✅ Blueprint looks good. Nice use of the conventional bp for the name.

anselrognlie · 2023-04-28T01:44:44Z

+    for planet in planets:
+        if planet.id == id:
+            return planet 
+    abort(make_response({"message": f"Cat with id {id} was not found"}, 404))


Update to Planet.

abort(make_response({"message": f"Planet with id {id} was not found"}, 404))

anselrognlie · 2023-04-28T01:53:13Z

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


👀 Watch out for stray lines. Decorators should sit directly on top of the function they're decorating.

anselrognlie · 2023-04-28T01:53:56Z

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


✅ Looks good. But since you added the helper to make a dict, consider using a list comprehension to build up the converted list of dictionaries.

anselrognlie · 2023-04-28T02:01:42Z

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


✅ Looks good. Consider naming this something like get_single_planet rather than using search. Search usually implies a more involved operation, like looking for matches by name or something, and often there might be multiple results. Here's we're retrieving by id, not so much searching.

anselrognlie

Nice job overall, though I suspect there may be some fallout from the GH confusion at the end. The __init__.py for the project didn't get updated to connect to the database. When I did so locally, there were a number of problems that prevented the tests you have from running properly. Consult my comments for details. Your code is pretty close to what we'd like to see, so just stay vigilant when working on Task List.

anselrognlie · 2023-05-07T20:21:19Z

It looks like the updates to connect to the database didn't make it in here. Maybe it was misplaced with the GH confusion? In addition to registering the routes, things that we'd want to see in this file include:

Initialize the database and migration systems

Load connection string

Load models (though if we load a route file that itself loads the models, we don't need to do this here)

anselrognlie · 2023-05-07T20:22:02Z

+
+class Planet(db.Model):
+    id = db.Column(db.Integer, primary_key=True, autoincrement=True)
+    name = db.Column(db.String, nullable=False)


👍 Nice decision to mark these columns as not nullable.

anselrognlie · 2023-05-07T20:23:58Z

+    @classmethod
+    def from_dict(cls, data_dict):
+        return cls(
+            id=data_dict["id"],


👀 For at least the scenario we're using from_dict now (POST create route), we would not set the id explicitly. We want the database to set this when we commit the model record.

anselrognlie · 2023-05-07T20:34:29Z

+            rating=data_dict["rating"]
+        )
+
+    def to_dictionary(self):


👀 As modelled in Learn (and how your code is calling it) this should be named to_dict.

anselrognlie · 2023-05-07T20:35:42Z

+from flask import abort, make_response
+
+#HELPER FUNCTIONS
+def validate_model(cls, id):


👀 This helper should return the model if it was able to be looked up.

anselrognlie · 2023-05-07T20:55:04Z

+
+# GET ONE ENDPOINT
+@bp.route("/<id>", methods=["GET"])
+def handle_planet(id):


anselrognlie · 2023-05-07T20:56:16Z

+    planet = validate_model(Planet, id)
+    request_body = request.get_json()
+
+    planet.id = request_body["id"]


👀 We should not attempt to change the id. Once the id has been assigned by the database during the create, we should leave it unchanged as long as the data remains in the DB.

anselrognlie · 2023-05-07T21:14:25Z

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


As with my suggestion for the POST endpoint, we may want to wrap this with a try/expect to handle any KeyError that occurs.

anselrognlie · 2023-05-07T21:15:55Z

+
+    db.session.commit()
+
+    return make_response(f"Planet {planet.name} successfully updated", 200)


👀 At the least, this should jsonify the result so that all our endpoints return JSON. However, as with the POST endpoint, we might want to return the updated record.

anselrognlie · 2023-05-07T21:16:46Z

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


👀 At the least, this should jsonify the result so that all our endpoints return JSON. However, we might consider having no response body at all, since the 200 status itself should be enough to tell the caller that the delete occurred successfully.

DQLIU1995 added 2 commits April 25, 2023 13:16

wave 1 and wave 2

ed54562

minor change

7cc6cf0

anselrognlie reviewed Apr 28, 2023

View reviewed changes

celina-barron added 2 commits May 5, 2023 14:56

all waves

3b93485

removed routes.py

2e73ba4

anselrognlie reviewed May 7, 2023

View reviewed changes



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


		db.session.commit()

		return make_response(f"Planet {planet.name} successfully updated", 200)



		@bp.route("", methods=["GET"])

Conversation

DQLIU1995 commented Apr 25, 2023

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants