C22 Izzy Hirad, Christelle Nkera , Marjana Khatun#13
C22 Izzy Hirad, Christelle Nkera , Marjana Khatun#13marjanak wants to merge 13 commits intoAda-C22:mainfrom
Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work y'all! I left a couple suggestions of things to take forward for future projects.
I like seeing that your team was making commits as you went, in the future I would like to see folks practicing writing more detailed commit messages that talk about the specific files changed, routes added, etc. (closer to the last couple commit messages that included some information on what changed in the commit).
| number_moons = request_body["number_moons"] | ||
| color = request_body["color"] | ||
|
|
||
| new_planet = Planet(name=name, description=description,number_moons=number_moons,color=color) |
There was a problem hiding this comment.
Another way to break this line up for length and readability could be:
new_planet = Planet(
name=name,
description=description,
number_moons=number_moons,
color=color
)| name = request_body["name"] | ||
| description = request_body["description"] | ||
| number_moons = request_body["number_moons"] | ||
| color = request_body["color"] |
There was a problem hiding this comment.
What kind of error could be raised by this code? What's a way we could handle it?
There was a problem hiding this comment.
This could raise a KeyError, and we could add a try/except?
| if number_moons_param: | ||
| query = query.where(Planet.number_moons >= 95 ) | ||
|
|
||
| query=query.order_by(Planet.id) |
There was a problem hiding this comment.
| query=query.order_by(Planet.id) | |
| query = query.order_by(Planet.id) |
| planet.name = request_body["name"] | ||
| planet.description = request_body["description"] |
There was a problem hiding this comment.
What other attributes does a Planet model have that we might want to include in a PUT update route?
There was a problem hiding this comment.
number_moons = request_body["number_moons"]
color = request_body["color"]
|
|
||
| } | ||
|
|
||
| def test_planet_not_found(client): |
There was a problem hiding this comment.
The name test_planet_not_found is ambiguous, because there are many scenarios across several of our routes where a planet might not be found (trying to delete a planet, trying to update a planet, trying to fetch a planet). The larger our project is, the more important it becomes to include the name of the function we're testing in the test name to make it clear at a glance exactly what our intention is.
def test_get_one_planet_not_found(client):5 participants
No description provided.