Ocelots - Lisa Utsett, Xuan Hien Pham#20
Conversation
…on that handles 400/404 errors
Planets models setup
…r by name and sort in either ascending or descending order.
… file from routes.py. Deleted routes.py. Added functionality that allows the user to sort by any column.
'to_dict_refactor' of https://github.com/lisabethu88/solar-system-api into to_dict_refactor
…s method for the Planet class
…olar-system-api into from_dict_refactor
…olar-system-api into from_dict_refactor
…system-api into nested_routes
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work Lisa & Chelsea! I've left a few comments & questions, feel free to reply here or message me on Slack if you have questions on the feedback 🙂
| description = db.Column(db.String) | ||
| name = db.Column(db.String) | ||
| planet_id = db.Column(db.Integer, db.ForeignKey('planet.id')) | ||
| planets = db.relationship("Planet", back_populates = "moons") |
There was a problem hiding this comment.
Does this attribute hold a single planet, or many planets? If it is only ever connected to 1 planet, I would suggest using a singular variable name to better describe the contents.
| moon_names = [] | ||
| for moon in self.moons: | ||
| moon_names.append(moon.name) | ||
| planet_dict["moons"] = moon_names |
There was a problem hiding this comment.
I know this is the way I grabbed cat names during the class livecode, but this is a great place for a list comprehension!
planet_dict["moons"] = [moon.name for moon in self.moons]| @@ -0,0 +1,32 @@ | |||
| from app import db | |||
There was a problem hiding this comment.
It looks like the route files are loose in the app folder. To keep a project organized as it grows, I recommend creating a routes folder inside of app to place these in.
| moons_response = [] | ||
| for moon in moons: | ||
| moons_response.append(moon.to_dict()) |
There was a problem hiding this comment.
This would be another great place for a list comprehension - what could that look like?
There was a problem hiding this comment.
moons_response = [moon.to_dict() for moon in moons]
| from sqlalchemy import desc, asc | ||
|
|
||
|
|
||
| # ---------helper functions-------- |
There was a problem hiding this comment.
Specific teams might have their own style guides, but it is a common practice to place helper functions at the end of a file rather than the beginning. An often-used analogy is to lay out code like a newspaper article. Headlines and the most important information comes first - Functions that use other functions in the file go at the top. That gets followed by the details and extra information in an article, or in our code, all the helper functions that support the higher up functions.
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Tiny best practices feedback - we typically only want a single blank line to separate functions in a file. If we have blocks of functions that do different things, then we might consider using 2 blank lines. A solid example of this are lines 37 and 38 where we have 2 blank lines separating the block of helper functions from the route functions that follow.
|
|
||
|
|
||
|
|
||
| def validate_model(cls, model_id): |
There was a problem hiding this comment.
Since this function is shared and not specific to the planet routes, I would suggest moving it out of the planet_routes.py file.
| # if "name" not in request_body or "description" not in request_body or 'diameter_in_km' not in request_body: | ||
| # return make_response("Invalid Request", 400) |
There was a problem hiding this comment.
Gentle reminder that we want to remove commented code as part of our review and clean up steps before opening PRs.
| model = cls.query.get(model_id) | ||
|
|
||
| if not model: | ||
| abort(make_response(jsonify({"message":f"{cls.__name__} #{model_id} not found"}), 404)) |
There was a problem hiding this comment.
Around best practices, there are many lines across the files that break the PEP 8 guide of 79 characters max per line. I would keep an eye out for this in the future, especially on lines where there are nested function calls.
| return app.test_client() | ||
|
|
||
| @pytest.fixture | ||
| def one_planet(app): |
There was a problem hiding this comment.
I suggest updating this name to reflect that the function also creates a moon.
3 participants
No description provided.