C22 Wei and Beenish by beenishali693 · Pull Request #10 · Ada-C22/solar-system-api

beenishali693 · 2024-10-21T18:55:39Z

Waves 1 and 2 of Solar System API

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, mostly related to formatting.

anselrognlie · 2024-10-24T22:47:02Z

coworking_agreement.md

Nice to see that you discussed your coworking needs and preferences!

anselrognlie · 2024-10-24T22:48:15Z

app/models/planet.py

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


Nit: include a space after commas in parameters

def __init__(self, id, name, description, galaxy):

Looking at your data, consider setting a default value for the galaxy parameter.

def __init__(self, id, name, description, galaxy="Milky Way"):

anselrognlie · 2024-10-24T22:50:21Z

app/models/planet.py

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


👍 Nice job incorporating this refactor from the live code.

anselrognlie · 2024-10-24T22:56:09Z

app/models/planet.py

+        return {
+                "id": self.id,
+                "name": self.name,
+                "description": self.description,
+                "galaxy": self.galaxy
+            }


Personally, I prefer to stick with uniform 4-space indented wrapping rather than trying to align to the enclosing character. Generally the closing brace should align with either the first line (pre-indent), or the content lines (1-level indent). As written, it's a bit floaty.

return { "id": self.id, "name": self.name, "description": self.description, "galaxy": self.galaxy }

anselrognlie · 2024-10-24T22:56:53Z

app/models/planet.py

+            }
+
+
+mercury = Planet(1,"Mercury","first planet from the sun","Milkyway")


👀 Space after argument commas here and throughout.

mercury = Planet(1, "Mercury", "first planet from the sun", "Milkyway")

anselrognlie · 2024-10-24T22:57:39Z

app/routes/planet_routes.py

@@ -0,0 +1,31 @@
+from flask import Blueprint, make_response, abort
+from ..models.planet import planets


👍 Nice relative import.

anselrognlie · 2024-10-24T22:58:07Z

app/routes/planet_routes.py

+from flask import Blueprint, make_response, abort
+from ..models.planet import planets
+
+planets_bp = Blueprint("planets_bp",__name__,url_prefix="/planets")


👀 Spaces after argument commas.

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

anselrognlie · 2024-10-24T22:59:03Z

app/routes/planet_routes.py

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


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

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

anselrognlie · 2024-10-24T22:59:43Z

app/routes/planet_routes.py

+@planets_bp.get("/<planet_id>")
+def get_one_planet(planet_id):
+    planet = validate_planet(planet_id)
+    return planet.to_dict(),200


The status code will be 200 by default, so we can leave that off.

return planet.to_dict()

anselrognlie

Nice work making the modifications to connect your API to your database, expanding the endpoints, and adding some tests. Everything here should be able to be applied to your task list implementations.

anselrognlie · 2024-11-04T00:57:03Z

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-04T00:57:18Z

app/__init__.py

    app = Flask(__name__)
+
+    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-04T00:57:37Z

app/__init__.py

@@ -1,9 +1,21 @@
 from flask import Flask
+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-04T01:02:18Z

app/models/planet.py

-        self.name = name
-        self.description = description
-        self.galaxy = galaxy
+class Planet(db.Model):


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

anselrognlie · 2024-11-04T01:02:52Z

app/routes/planet_routes.py

+    name = request_body["name"]
+    description = request_body["description"]
+    galaxy = request_body["galaxy"]


We didn't explicitly look at invalid/missing data scenarios for creating a record, but think about what would happen here, and what response we might want to reply with.

anselrognlie · 2024-11-04T01:26:43Z

tests/conftest.py

+    galaxy="Milky Way")
+
+    db.session.add_all([earth,mars])
+    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 and 2, we could use the ids actually assigned by the database to remove that assumption.

anselrognlie · 2024-11-04T01:33:49Z

tests/test_planet_routes.py

+    response = client.get("/planets/1")
+
+    #Assert
+    assert response.status_code == 404


👀 We should also check that the response body is the expected message.

anselrognlie · 2024-11-04T01:34:36Z

tests/test_planet_routes.py

+
+def test_get_one_planet_with_records_successful(client, two_saved_planets):
+    #Act
+    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/{two_saved_planets[0].id}")

anselrognlie · 2024-11-04T01:35:13Z

tests/test_planet_routes.py

+    #Assert
+    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": two_saved_planets[0].id,

anselrognlie · 2024-11-04T01:38:46Z

tests/test_planet_routes.py

+    count_response = client.get("/planets/count")
+    response_body = count_response.get_json()


While this will show that the number of planets has decreased, we'd really like to check that planet id 1 is actually the one that's gone. So rather than getting the count, we might try to get planet 1. After deletion, we should get a 404 result.

We could write a separate test for the count endpoint.

hintow and others added 16 commits October 18, 2024 11:35

aggrement

ad158b5

"added signature to coworking_agreement"

597d34c

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

cb8eb34

draft wave 1&2

f1433e4

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

9856a3f

draft 2

c02f9bf

"End of Wave 1"

dc1f1fa

End of wave 1

968179e

rename planets to planet

c9c7631

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

93abc66

Wave 2 completed

9e3d33e

moved to_dict to planets file

65111b2

merge wave2

2c4dedc

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

5310ed1

solve to_dict problem

a8a5c53

200 response code

b11ec73

anselrognlie reviewed Oct 24, 2024

View reviewed changes

beenishali693 and others added 13 commits October 28, 2024 17:09

finished with wave 3

0fdc1a2

get one planet

5dee1a8

added get_one_planet, update and delete routes

e949917

wave4

69d4628

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

49c96a6

wave 5

fc70d52

added seed.py

410610c

added a count parameter

d49aaed

wave 6

85cd85d

end of wave 6

5f2e3af

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

5d3a50a

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

c63548a

minor changes

411075f

beenishali693 added 2 commits October 31, 2024 14:47

fixed delete test to also check the count

d0c56c7

added test on learn

e611944

anselrognlie reviewed Nov 4, 2024

View reviewed changes

		}


		mercury = Planet(1,"Mercury","first planet from the sun","Milkyway")

		@@ -0,0 +1,31 @@
		from flask import Blueprint, make_response, abort
		from ..models.planet import planets



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

		count_response = client.get("/planets/count")
		response_body = count_response.get_json()

Conversation

beenishali693 commented Oct 21, 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!

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