Sphinx-Weixi_He, Phoenix- Madina Dzhetegenova #67
Conversation
Madina-j
changed the title
| from flask import Blueprint, abort, make_response | ||
| from ..models.planet import planets | ||
|
|
||
| solar_system_bp = Blueprint("solar_system_bp", __name__, url_prefix="/planets") |
There was a problem hiding this comment.
Prefer to use the resource name (rather than the project name) as part of the blueprint name. So here, planets_bp, since this blueprint is related to the planets resources.
There was a problem hiding this comment.
When moving the routes file into a folder (theoretically, to group multiple routes files together as we add additional resources), prefer to include the resource name as part of the file name. So here, something like planets_routes.py or planet_routes.py.
app/models/planet.py
Outdated
| @@ -0,0 +1,19 @@ | |||
|
|
|||
| class Planet: | |||
| def __init__(self, id, name, description, flag= bool): | |||
There was a problem hiding this comment.
👀 The expression for the default would generally be a value of the desired type, rather than the type name itself.
def __init__(self, id, name, description, flag=False):As written, if we left of the flag value, then self.flag would be literally set to the bool type, not a boolean value (True/False).
There was a problem hiding this comment.
It's not immediately obvious that the attrbiute called flag is intended to store a boolean. Consider a name like has_flag. Variables that start with words like is_ or has_ (depending on what makes grammatical sense) make it more clear that this is intended to hold a yes/no (boolean) value.
app/models/planet.py
Outdated
| Planet(3, "Earth", "The only planet known to support life.", True ), | ||
| Planet(4, "Mars", "Known as the Red Planet.", True ), |
There was a problem hiding this comment.
Nit: be careful of extra spaces.
Planet(3, "Earth", "The only planet known to support life.", True),
Planet(4, "Mars", "Known as the Red Planet.", True),
app/routes/routes.py
Outdated
| @@ -0,0 +1,42 @@ | |||
| from flask import Blueprint, abort, make_response | |||
| from ..models.planet import planets | |||
There was a problem hiding this comment.
👍 Nice relative import.
app/routes/routes.py
Outdated
| return{ | ||
| "id":planet.id, | ||
| "name":planet.name, | ||
| "description":planet.description, | ||
| "flag":planet.flag | ||
| } |
There was a problem hiding this comment.
Nit: for dictionary literals, prefer spaces after the :
return {
"id": planet.id,
"name": planet.name,
"description": planet.description,
"flag": planet.flag
}
app/routes/routes.py
Outdated
| results_list.append(dict( | ||
| id=planet.id, | ||
| name=planet.name, | ||
| description=planet.description, | ||
| flag=planet.flag | ||
| )) |
There was a problem hiding this comment.
Even though this is using the dict() style and later (in get_one_planet) we use the {} style, notice how both are building the same dictionary structure from a planet record. Maybe we could make a helper function/method to DRY up our code.
|
|
||
| def create_app(): | ||
| def create_app(config=None): |
There was a problem hiding this comment.
👍 We need this param to accept the additional options for testing, but it needs to be defaulted since nothing will be passed in when starting with flask run
| app = Flask(__name__) | ||
|
|
||
| app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False | ||
| app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get('SQLALCHEMY_DATABASE_URI') |
There was a problem hiding this comment.
👍 Loads the connection string from the environment (provided with our .env during development).
|
|
||
| if config: | ||
| app.config.update(config) | ||
| db.init_app(app) |
There was a problem hiding this comment.
Nit: Leave a blank line between the end of the conditional block and this logic.
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| name: Mapped[str] | ||
| description: Mapped[str] | ||
| flag:Mapped[bool] |
There was a problem hiding this comment.
Nit: Leave a space after the : in type expressions.
flag: Mapped[bool]| self.name = name | ||
| self.description = description | ||
| self.flag = flag | ||
| class Planet(db.Model): |
There was a problem hiding this comment.
👍 Nice modifications to associate this model type with our database.
| return app.test_client() | ||
|
|
||
| @pytest.fixture | ||
| def two_saved_planets(app): |
There was a problem hiding this comment.
👍 Nice test fixture adding two records.
|
|
||
| db.session.add_all([planet_1, planet_2]) | ||
|
|
||
| db.session.commit() |
There was a problem hiding this comment.
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.
|
|
||
| def test_get_one_book_succeeds(client, two_saved_planets): | ||
| # Act | ||
| response = client.get("/planets/1") |
There was a problem hiding this comment.
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}")| # Assert | ||
| assert response.status_code == 200 | ||
| assert response_body == { | ||
| "id": 1, |
There was a problem hiding this comment.
If we returned the record list from the fixture, then we could check the id as
"id": two_saved_planets[0].id,|
|
||
| def test_get_planet_by_id_returns_404(client): | ||
| response = client.get("/planets/1") | ||
| assert response.status_code == 404 |
There was a problem hiding this comment.
👀 We should also check that the response body is the expected message.
No description provided.