Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
👍 Keep it up for Task List API!
Try to apply all the refactors we went over in Learn and class as you work on Task List.
| migrate = Migrate() | ||
| load_dotenv() | ||
|
|
||
| def create_app(test_config=None): |
| description = db.Column(db.Text, nullable=False) | ||
| rings = db.Column(db.Boolean, nullable=False) | ||
|
|
||
| def build_planet_dict(self): |
There was a problem hiding this comment.
Prefer to leave planet out of the name of this function, so build_dict, or as we used in RT, to_dict. Since this is a method of Planet, it's a little redundant to have the class' own name in the methd. We probably will have an instance in a variable named something like planet, in which case, calling this would look like planet.build_planet_dict(). If we shorten the name, it would be planet.build_dict(), or planet.to_dict(), which already communicates that we're converting a planet to a dictionary representation.
| @@ -0,0 +1,5 @@ | |||
| <<<<<<< HEAD | |||
There was a problem hiding this comment.
It looks like this file (which Alembic should have generated itself) somehow ended up with a merge conflict in it? Not sure what happened here.
| import logging | ||
| from logging.config import fileConfig | ||
|
|
||
| <<<<<<< HEAD |
There was a problem hiding this comment.
There are more merge conflcicts here. At some point did your migrations stop working? These conflicts look like they would cause the code in this file to break.
| @@ -0,0 +1,34 @@ | |||
| """adds Book model | |||
| Planet(name="Mars", description="The red planet", rings=False), | ||
| Planet(name="Jupiter", description="The gas giant", rings=False), | ||
| Planet(name="Saturn", description="The second largest planet", rings=True), | ||
| Planet(name="Uranus", description="This planet spins on its side", rings=True), |
There was a problem hiding this comment.
👍 The fact that Uranus spins on its side is why it's my favorite planet!
|
|
||
| planet_bp = Blueprint("planets", __name__, url_prefix="/planets") | ||
|
|
||
| def validate_planet(planet_id): |
There was a problem hiding this comment.
👀 Consider refactoring this to receive the class type as a parameter so that our validate helper could work for more than one type. This will be helpful in Task List when adding the Goal model.
|
|
||
| planet_query = Planet.query | ||
|
|
||
| if name_query: |
There was a problem hiding this comment.
👍 Nice handling of multiple query filters.
| db.session.add(new_planet) | ||
| db.session.commit() | ||
|
|
||
| return make_response(f"Planet {new_planet.name} successfully created", 201) |
There was a problem hiding this comment.
👀 Make sure to jsonify even plain string responses, otherwise flask will return an HTML response rather than JSON.
| planet = validate_planet(planet_id) | ||
| request_body = request.get_json() | ||
|
|
||
| planet.name = request_body["name"] |
There was a problem hiding this comment.
Here (and in the create method) consider handling a possible KeyError with a more tailored message (similar to the abort handling in the validation function).
3 participants
No description provided.