Sea Turtles - Esther A. & Danielle W.#11
Sea Turtles - Esther A. & Danielle W.#11estherannorzie wants to merge 10 commits intoada-c17: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've left a few comments, please reach out if you have any questions on the feedback 🙂
|
|
||
| return jsonify(to_dict(planet)) | ||
|
|
||
| def validate_planet(planet_id): |
There was a problem hiding this comment.
I'd consider renaming this function. To me, validate_planet sounds like it should return a true/false based on if the planet passed as a parameter is valid. What name might better describe what the function is doing?
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Nice looking code Esther & Danielle! I've left a few comments, feel free to reply here or message me on Slack if you have questions on the feedback 🙂
| app.config['SQLALCHEMY_DATABASE_URI'] = 'postgresql+psycopg2://postgres:postgres@localhost:5432/solar_system_development' | ||
| else: | ||
| app.config["TESTING"] = True | ||
| app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False |
There was a problem hiding this comment.
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 = Planet.query.get(planet_id) | ||
|
|
||
| if not planet: | ||
| abort(make_response({"message": f"planet id {planet_id} not found"}, 404)) |
There was a problem hiding this comment.
For a little extra code reuse, the abort calls in this function could also use the helper jsonify_message:
abort(jsonify_message({"message": f"planet id {planet_id} not found"}, 404))|
|
||
| db.session.commit() | ||
|
|
||
| return jsonify_message(f"Planet succesfully updated. {planet.to_dict()}") |
There was a problem hiding this comment.
I'd consider returning the planet name in the message like in create_planet, or if you want to return all the data for the planet, maybe separating that into a different key/value pair in the response. Placing the dictionary contents inside a message string sounds like it might not be easy to read.
3 participants
No description provided.