Sea Turtles, Christina & Jae #16

crw-b · 2022-04-26T20:20:37Z

No description provided.

anselrognlie

Looks good so far! I'd just suggest a little reorganization of the route file to its own folder. Also, is it just me, or some of the indents especially wide? 🤔

anselrognlie · 2022-04-27T05:24:18Z

app/planet_routes.py

@@ -0,0 +1,52 @@
+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-27T05:24:50Z

app/__init__.py

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


👍

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

anselrognlie · 2022-04-27T05:26:24Z

app/planet_routes.py

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


👍

Instance method for building a dictionary from an instance looks good!

anselrognlie · 2022-04-27T05:27:35Z

app/planet_routes.py

+    Planet(1, "Methuselah", "oldest exoplanet"),
+    Planet(2, "Epsilon Eridani b", "closest exoplanet, has 2 astroid belts!!"),
+    Planet(3, "Aquarii A", "the most suns!"),
+    Planet(4, "Gliese 876 b", "gas giant, big, icy moons, water might be found", False)


Read it here first: it's False that there's no life on Gliese 876b!

Although we'll be getting rid of this collection soon, 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 the other params, especially the bare boolean, aren't as obvious. Having those as named arguments would make it explicit.

anselrognlie · 2022-04-27T05:29:37Z

app/planet_routes.py

+    Planet(4, "Gliese 876 b", "gas giant, big, icy moons, water might be found", False)
+]
+
+bp = Blueprint("planets", __name__, url_prefix="/planets")


anselrognlie · 2022-04-27T05:30:15Z

app/planet_routes.py

+bp = Blueprint("planets", __name__, url_prefix="/planets")
+
+@bp.route("", methods=["GET"])
+def index_planets(): 


👍

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.

anselrognlie · 2022-04-27T05:32:54Z

app/planet_routes.py

+
+	return jsonify(result_list) 
+
+def validate_planet(id):


👍

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

anselrognlie · 2022-04-27T05:33:35Z

app/planet_routes.py

+
+
+@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.

anselrognlie

Nice job finishing up solar system, and adding so many additional tests! 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-10T00:46:35Z

app/__init__.py



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

-    return app
+    if not test_config:
+        app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False


Notice that since this option is being set in both branches of the if, it could be pulled out of the branches to either before or after the if to remove the repetition.

anselrognlie · 2022-05-10T00:47:05Z

app/__init__.py

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


Once we have an import path that leads to the model being imported (init→route→model) we don't really need this import here, but it can be handy as a sanity check.

anselrognlie · 2022-05-10T00:53:24Z

app/models/planet.py

+    name = db.Column(db.String)
+    description = db.Column(db.String)
+    life = db.Column(db.Boolean)
+    moons = db.Column(db.Integer)


Consider setting some of these columns to have nullable=false. For example, should we be able to create a planet without a name? Should life be able to be NULL rather than true or false? If there are no moons, shouldn't we require the value to be 0 rather than NULL?

anselrognlie · 2022-05-10T00:53:35Z

app/models/planet.py

+    life = db.Column(db.Boolean)
+    moons = db.Column(db.Integer)
+
+    def to_dict(self):


anselrognlie · 2022-05-10T00:53:44Z

app/models/planet.py

+        )
+
+    @classmethod
+    def from_dict(cls, data_dict):


anselrognlie · 2022-05-10T01:16:28Z

app/routes/planet_routes.py

+	result = replace_planet_safely(planet, request_body)
+	db.session.commit()
+	return jsonify(result)


I'd be more inclined to have the replace calls just update the contents without returning the dictionarified result. More like the following.

replace_planet_safely(planet, request_body) db.session.commit() return jsonify(planet.to_dict())

anselrognlie · 2022-05-10T01:18:00Z

app/routes/planet_routes.py

+def upgrade_planet_with_id(id):
+	planet = validate_planet(id)
+	request_body = request.get_json()
+	planet_keys = request_body.keys()


We don't need to retrieve the keys separately here. The in operator on dictionaries checks for a key itself, so the later checks could be written as:

if "name" in request_body: # handle name key...

anselrognlie · 2022-05-10T01:18:36Z

app/routes/planet_routes.py

+	if "name" in planet_keys:
+		planet.name = request_body["name"]
+	if "description" in planet_keys:
+		planet.description = request_body["description"]
+	if "life" in planet_keys:
+		planet.life = request_body["life"]
+	if "moons" in planet_keys:
+		planet.moons = request_body["moons"]


Consider moving this to an instance method in Planet.

anselrognlie · 2022-05-10T01:21:23Z

tests/test_routes.py

+def test_delete_planet(client, two_planets):
+
+    response = client.delete("/planets/1")
+    response_body = response.get_data(as_text=True)


Rather than having to handle this response specially, I'd suggest jsonfying the string that gets returned so that it is available through get_json() like all our other responses. We are writing a JSON web api, so we should be consistent about returning JSON results.

anselrognlie · 2022-05-10T01:22:32Z

app/routes/planet_routes.py

+	planet = validate_planet(id)
+	db.session.delete(planet)
+	db.session.commit()
+	return make_response('*** You have successfully destroyed Earth ! ***')


Make sure to return this as JSON. If we return a bare string, Flask will assume we want to return it as HTML. Also, make sure not to hardcode the name of the planet being destroyed.

return jsonify(f'*** You have successfully destroyed {planet.name} ! ***')

jaekerrclayton and others added 6 commits April 22, 2022 13:11

planets info

ea3df1c

wave 2 completed

db32efa

delete some unnecessary comments

7c5a72b

routes deleted

367ffbf

empty commit

cdff1bc

fixed?

61b6335

anselrognlie reviewed Apr 27, 2022

View reviewed changes

crw-b and others added 18 commits April 29, 2022 15:09

create planet route

1accee4

add get planet by id

465bf2a

add delete & put

2ceb860

somewhat refactored

4916a24

tests

d5cfa9a

update to init based on learn

f334f79

moved tests folder

1982623

passing 2 tests

a1956e4

made more tests

b4a0969

at least one test for everything except patch

fa1cf44

added patch, and 2 tests

a33ba3d

added moons to patch

8d25445

added patch test?

2089ea9

update tests

7981518

added tests for coverage

47e9242

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

75f32ec

update planet schema, add test for filter

e16bbb4

deleted a line

ad8ab8a

anselrognlie reviewed May 10, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sea Turtles, Christina & Jae #16

Sea Turtles, Christina & Jae #16

crw-b 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 Apr 27, 2022

anselrognlie left a comment

anselrognlie May 10, 2022

anselrognlie May 10, 2022

anselrognlie May 10, 2022

anselrognlie May 10, 2022

anselrognlie May 10, 2022

anselrognlie May 10, 2022

anselrognlie May 10, 2022

anselrognlie May 10, 2022

anselrognlie May 10, 2022

anselrognlie May 10, 2022

		@@ -0,0 +1,52 @@
		from flask import Blueprint, jsonify, abort, make_response

Sea Turtles, Christina & Jae #16

Are you sure you want to change the base?

Sea Turtles, Christina & Jae #16

Conversation

crw-b 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment