-
Notifications
You must be signed in to change notification settings - Fork 20
Tatiana-Lorraine-C22-Sphinx #6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
app/routes.py
Outdated
try: | ||
planet_id = int(planet_id) | ||
except: | ||
response = {"messange" : f"planet id {planet_id} is invalid"} | ||
abort(make_response(response, 400)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could avoid the need to use try ... except
block here by specifying a converter in the URL like this:
@planet_bp.get("/<planet_id>")
But in order to follow the instructions of the lesson, we kept the try ... except
block here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, you two! ✅
class Planet(db.Model): | ||
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
name: Mapped[str] | ||
description: Mapped[str] = mapped_column(nullable=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its more preferred to use Optional
here instead of nullable
. https://docs.sqlalchemy.org/en/20/orm/declarative_tables.html#using-annotated-declarative-table-type-annotated-forms-for-mapped-column
allowed_sort_fields = { | ||
"name": Planet.name, | ||
"radius_in_mi": Planet.radius_in_mi, | ||
} | ||
|
||
sort_param = request.args.get("sort") | ||
if sort_param: | ||
sort_param = sort_param.split(" ") | ||
# Selects the field for sorting based on the user's input (`sort_param[0]`). | ||
# If no valid sort field is specified, defaults to sorting by Planet.id. | ||
sort_field = allowed_sort_fields.get(sort_param[0].lower(), Planet.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we know about hasattr
and getattr
, how could this be rewritten?
request_body = request.get_json() | ||
|
||
planet.name = request_body["name"] | ||
planet.description = request_body["description"] | ||
planet.radius_in_mi = request_body["radius_in_mi"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be its own helper function?
planet = validate_model(Planet, planet_id) | ||
db.session.delete(planet) | ||
db.session.commit() | ||
|
||
return Response(status=204, mimetype="application/json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏿
request_body = request.get_json() | ||
for planet_data in request_body["planets"]: | ||
create_planet_helper(planet_data) | ||
|
||
return Response(status=201, response="Planets have been created!", mimetype="application/json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
return Response( | ||
status=200, | ||
response=f"All {num_rows_deleted} planets have been deleted!", | ||
mimetype="application/json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also be done with a regular dictionary.
db.session.add(new_planet) | ||
db.session.commit() | ||
|
||
return new_planet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! 👍🏿
@pytest.fixture | ||
def client(app): | ||
return app.test_client() | ||
|
||
@pytest.fixture | ||
def two_saved_planets(app): | ||
# Arrange | ||
Mercury = Planet( | ||
name="Mercury", | ||
description="about 1/3 the size of Earth", | ||
radius_in_mi=1.516 | ||
) | ||
Venus = Planet( | ||
name="Venus", | ||
radius_in_mi=3.760, | ||
description="only slightly smaller than Earth" | ||
) | ||
|
||
db.session.add_all([Mercury, Venus]) | ||
db.session.commit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨
"name": "Earth", | ||
"description": "Earth itself", | ||
"radius in miles" : 3.959 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tests, they are robust!
# cause an `HTTPException` when an `abort` statement is reached | ||
with pytest.raises(HTTPException): | ||
result_Planet = validate_model(Planet, "cat") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐️
No description provided.