C22, Alejandra and Maybellene, part 1 by mayboolean · Pull Request #1 · Ada-C22/solar-system-api

mayboolean · 2024-10-18T19:06:52Z

No description provided.

anselrognlie

You're off to a good start. All the Flask-specific code is on the right on track. There are just a few general Python points to review.

anselrognlie · 2024-10-24T05:19:17Z

app/models/planet.py

@@ -0,0 +1,20 @@
+class Planet:
+    def __init__(self, id, name, description, size_dia_km):


👍 Great idea to include more specifics about what "size" means (the diameter in kilometers). This is often useful to do with a data value that's otherwise just a number.

anselrognlie · 2024-10-24T05:19:51Z

app/models/planet.py

+        self.id = id
+        self.name = name
+        self.description = description
+        self.size = size_dia_km


Consider carrying the additional detail in the name of the attribute itself.

anselrognlie · 2024-10-24T05:20:20Z

app/models/planet.py

+        self.description = description
+        self.size = size_dia_km
+
+    def to_dict(self):


👍 Nice job incorporating this refactor from the live code.

anselrognlie · 2024-10-24T05:21:00Z

app/models/planet.py

+        return dict(
+            id=self.id,
+            name=self.name,
+            desc=self.description,


Unless there's a strong need to, resist the temptation to shorten the name in the dictionary-ified version.

anselrognlie · 2024-10-24T05:21:25Z

app/models/planet.py

+planets = [
+    Planet(1,"some_planet", "rocky, no signs of life", 100),
+    Planet(2, "Earth", "with lifeforms, water and land", 5000),
+    Planet(3, "Pluto", "still a planet", 200)


Disagree. 😆

anselrognlie · 2024-10-24T05:23:02Z

app/routes/planet_routes.py

+from app.models.planet import planets
+
+# created blueprint
+planets_bp = Blueprint("planets_bp",__name__, url_prefix=("/planets"))


There's no need to wrap the url_prefix value in ().

Be sure to include a space after ,

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

anselrognlie · 2024-10-24T05:23:50Z

app/routes/planet_routes.py

+    results_list = []
+
+    for planet in planets: 
+        results_list.append(planet.to_dict())


There's a great opportunity to try to use a list comprehension here.

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

anselrognlie · 2024-10-24T05:24:58Z

app/routes/planet_routes.py

+        planet_id = int(planet_id)
+    except ValueError:
+        abort(make_response({"message": f"planet {planet_id} is an invalid ID"}, 400))
+    for planet in planets:


Consider adding a blank line between this line and the previous to help separate these two sections of code that focus on two different things.

anselrognlie · 2024-10-24T05:28:45Z

app/routes/planet_routes.py

+    else:
+        abort(make_response({"message": f"planet {planet_id} not found"}, 404))


Python does support else blocks on loops, but as a syntax pattern that's relatively uncommon, I generally avoid it. It's also not necessary here. An else block is run if a loop runs all the way to completion. Since the only way the loop wouldn't run to completion is by finding (and returning) the desired planet, the only way we can reach this code at all is if the loop runs to completion.

We can simplify this by removing the else: an unindenting its block.

anselrognlie

Nice work making the modifications to connect your API to your database, expanding the endpoints, and adding some tests. Make sure you're committing any additional migrations (it looks like you update the planet model, but the migration still represents an older schema). Otherwise, everything here should be able to be applied to your task list implementations.

anselrognlie · 2024-11-03T21:57:26Z

app/__init__.py


-
-def create_app(test_config=None):
+def create_app(config=None):


👍 Thanks for renaming this. This param could be used for scenarios other than testing (the name was left over from the previous curriculum).

anselrognlie · 2024-11-03T22:01:38Z

app/__init__.py

 from flask import Flask
 from .routes.planet_routes import planets_bp
+from .db import db, migrate
+from .models import planet


Note that once we have the Planet model imported in the routes, and the routes are imported here, we don't technically need to import the planet module here. If you find the fact that VS Code shows this as not being used, it would now be safe to remove.

anselrognlie · 2024-11-03T22:02:10Z

app/__init__.py

-    # registered the planets blueprint with the app
+
+    app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
+    app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get('SQLALCHEMY_DATABASE_URI')


👍 Loads the connection string from the environment (provided with our .env during development).

anselrognlie · 2024-11-03T22:03:44Z

app/models/planet.py

+from sqlalchemy.orm import Mapped, mapped_column
+from ..db import db
+
+class Planet(db.Model):


👍 Nice modifications to associate this model type with our database.

anselrognlie · 2024-11-03T22:14:02Z

app/routes/planet_routes.py

+def create_planet():
+    request_body = request.get_json()
+    name = request_body["name"]
+    description= request_body["description"]


Nit: Make sure to include spaces around assignments

description = request_body["description"]

anselrognlie · 2024-11-03T22:25:53Z

tests/conftest.py

+
+
+@pytest.fixture
+def three_saved_planets(app):


👍 Nice test fixture adding three records.

anselrognlie · 2024-11-03T22:26:45Z

tests/conftest.py

+    planet3 = Planet(name="Pluto", description="still a planet", size_dia_km=200)
+
+    db.session.add_all([planet1, planet2, planet3])
+    db.session.commit()


Consider returning the list of records. The return value becomes the value passed in for the fixture in a test. By returning the records, we could use their ids in the dependent tests. While our approach of dropping and recreating the database for each test should guarantee that these records are ids 1, 2, and 3, we could use the ids actually assigned by the database to remove that assumption.

anselrognlie · 2024-11-03T22:28:19Z

tests/test_planet_routes.py

+
+
+def test_get_single_planet(client, three_saved_planets):
+    response = client.get("/planets/1")


If we returned the record list from the fixture, then we could write this line as

response = client.get(f"/planets/{three_saved_planets[0].id}")

anselrognlie · 2024-11-03T22:29:40Z

tests/test_planet_routes.py

+
+    assert response.status_code == 200
+    assert response_body == {
+        "id": 1,


If we returned the record list from the fixture, then we could check the id as

"id": three_saved_planets[0].id,

anselrognlie · 2024-11-03T22:30:36Z

tests/test_planet_routes.py

+    response = client.get("/planets/1")
+    response_body = response.get_json()
+
+    assert response.status_code == 404


👀 Be sure to check that the expected response body is also returned.

mayboolean and others added 8 commits October 18, 2024 14:19

define fodler struct

73a4709

created planet class

ca1b4b4

wave 01

053b336

work and edit

26925b3

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

8bb3877

We merged it

21cbdf4

wave 2

fa09142

Implements helper function and class method

6750914

anselrognlie reviewed Oct 24, 2024

View reviewed changes

222alle111 and others added 7 commits October 25, 2024 14:57

Wave 3 still in process

85eca8c

Wave 4 add PUT and DELETE

e07c7df

Wave 5 query params name, description, size added

22dcd80

unfinished

e8ae877

finishes wave 5

921ec4f

changes size to size_dia_km

bd9d357

implemented more of ansel's review from wave 1 and 2

6587d4d

anselrognlie reviewed Nov 3, 2024

View reviewed changes

		@@ -0,0 +1,20 @@
		class Planet:
		def __init__(self, id, name, description, size_dia_km):

		else:
		abort(make_response({"message": f"planet {planet_id} not found"}, 404)) No newline at end of file



		def create_app(test_config=None):
		def create_app(config=None):



		def test_get_single_planet(client, three_saved_planets):
		response = client.get("/planets/1")

Conversation

mayboolean commented Oct 18, 2024

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