Kunzite - Abby & Aisha#24
Conversation
…ote handle_planets(), and moved the GET planet_id route next to handle_planet()
marciaga
left a comment
marciaga
left a comment
There was a problem hiding this comment.
Good work with Waves 01-02! Please see specific comments below.
| "id": planet.id, | ||
| "name": planet.name, | ||
| "description": planet.description, | ||
| "chemical composition": planet.composition |
There was a problem hiding this comment.
Notice here the key being used is "chemical composition" but in the Planet class, the attribute is called composition. It's very important to keep these attributes/keys consistent because any clients who consume this data as returned from your API will be relying on that uniformity. I'd recommend sticking with composition or if you prefer a compound word, use the Python convention of an underscore, i.e. chemical_composition.
|
|
||
| abort(make_response({"message": f"planet {planet_id} not found"}, 404)) | ||
|
|
||
| def make_dict(planet): |
There was a problem hiding this comment.
Following what we did in Flasky, it's advisable to make this function an instance method in your Planet class, so you can call it on instances of the class!
|
|
||
| def make_dict(planet): | ||
| return dict( | ||
| id = planet.id, |
There was a problem hiding this comment.
within dict(), the convention is to not have spaces around the assignment operators. It's perhaps more important to be consistent with the approach, so if you're using spaces around assignment operators, do that everywhere you use it. But the PEP8 preference here is to omit the spaces, so
dict(
id=planet.id,
name=planet.name,
description=planet.description,
composition=planet.composition
)
marciaga
left a comment
marciaga
left a comment
There was a problem hiding this comment.
Great work! Only thing worth mentioning is to avoid committing commented out code. Nice job with the tests as well!
| def create_app(test_config=None): | ||
| app = Flask(__name__) | ||
|
|
||
| # app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False |
There was a problem hiding this comment.
It's a good practice to remove extraneous commented out code.
| "SQLALCHEMY_DATABASE_URI") | ||
| else: | ||
| app.config["TESTING"] = True | ||
| app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False |
There was a problem hiding this comment.
Since this line of code is in both the else and the if blocks, it could be moved outside this control flow structure so we don't repeat it.
| @@ -0,0 +1,23 @@ | |||
| from app import db | |||
|
|
|||
| class Planet(db.Model): | |||
| bp = Blueprint("planets", __name__, url_prefix="/planets") | ||
| # @bp.route("",methods=["GET"]) | ||
|
|
||
| # # helper function |
There was a problem hiding this comment.
Again, get in the habit of deleting commented out code.
| composition="second") | ||
|
|
||
| db.session.add_all([first_planet, second_planet]) | ||
| db.session.commit() No newline at end of file |
There was a problem hiding this comment.
This will work to get these planets in the db but if you want to use the data in your tests, you should return something from this function (that will be passed to the test function). You could then use some of that test data instead of hard-coding. For example if you had a one_saved_planet fixture, you could use its id in the test_get_first_planet instead of hard-coding the get("planets/1").
3 participants
No description provided.