-
Notifications
You must be signed in to change notification settings - Fork 61
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
Sea Turtles - Adriana & Olive - Solar System #26
base: main
Are you sure you want to change the base?
Conversation
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.
Test
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.
Hey folks, I'm sorry my comments weren't published when I said they would be! Great work on part 1, I've left a few comments, please reach out if you have any questions on the feedback 🙂
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.
Great looking code Adriana & Olive! I've left a few comments & questions, feel free to reply here or message me on Slack if you have questions on the feedback 🙂
"SQLALCHEMY_DATABASE_URI") | ||
else: | ||
app.config["TESTING"] = True | ||
app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False |
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.
The line app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
is duplicated in both parts of the if/else. We could move that line to either above or below the if/else, so it only needs to be written once.
planet_dict = dict( | ||
id = self.id, | ||
name = self.name, | ||
description = self.description, | ||
gravity = self.gravity | ||
) | ||
return planet_dict |
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 is perfectly valid, but we could also return the dictionary without storing it in a variable like in from_dict
below.
if not result_list: | ||
return jsonify("No planets found with that name.") |
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.
Hmm, what happens if you try to get all planets, but there are no planets stored in the db yet? It looks like result_list
would be an empty list after line 44, so we'd enter the if-block on line 46 and print the error "No planets found with that name."
even though it doesn't quite apply. How could we make sure that message is only returned when relevant?
except KeyError as err: | ||
error_message(f"Missing key: {err}", 400) | ||
|
||
def replace_planet_safely(planet, data_dict): |
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.
Love the helper functions ^_^
return jsonify(f"Planet #{planet.id} successfully deleted.") | ||
|
||
@planets_bp.route("/<id>", methods = ["PATCH"]) | ||
def update_planet_with_id(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.
Nice patch function!
assert response.status_code == 201 | ||
assert response_body == "Planet Mercury successfully created" |
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 checks for both the status code and relevant response data across the tests!
No description provided.