Conversation
marciaga
left a comment
marciaga
left a comment
There was a problem hiding this comment.
Good work on Waves 01-02! I just left one comment below.
| def handle_planet(planet_id): | ||
| planet = validate_planet(planet_id) | ||
|
|
||
| return { |
There was a problem hiding this comment.
I'd recommend creating a make_dict instance method on the Planet class similar to how we did it in Flasky. That way you won't have to manually create dictionaries every time you want to return a Planet instance.
marciaga
left a comment
marciaga
left a comment
There was a problem hiding this comment.
Nice work! Way to add additional testing as well!
| "SQLALCHEMY_DATABASE_URI") | ||
| else: | ||
| app.config["TESTING"] = True | ||
| app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False |
There was a problem hiding this comment.
this line of code appears in both the if and else blocks so it could be extracted and moved outside this control flow, so the line doesn't get repeated.
| bp = Blueprint("planets", __name__, url_prefix="/planets") | ||
|
|
||
| # VALIDATE PLANET HELPER FUNCTIONS | ||
| def validate_model(cls, model_id): |
There was a problem hiding this comment.
Even though there's only one model in this codebase, this function is generic and could be used with any model, so it would be ideal to move it into its own helpers file.
| def read_all_planets(): | ||
| name_query = request.args.get("name") | ||
|
|
||
| if name_query: |
There was a problem hiding this comment.
Nice job adding the ability to pass the name query string parameter!
|
|
||
| def test_to_dict_no_missing_data(): | ||
| # Arrange | ||
| test_data = Planet(id = 2, |
| assert response_body == [] | ||
|
|
||
| # TEST ONE PLANET | ||
| def test_get_one_planet(client, two_saved_planets): |
There was a problem hiding this comment.
If you added a single saved planet fixture, you could use that to obtain the id instead of hard-coding it like below
| # Assert | ||
| assert new_planet.name == "New planet" | ||
| assert new_planet.description == "The mild planet" | ||
| assert new_planet.position == "#40" No newline at end of file |
There was a problem hiding this comment.
Thorough model tests! I think for this last one, if you want to make sure it doesn't contain any additional key/values, you should assert that the new_planet dictionary is identical with the key/values you have on separate lines, since new_planet might contain key/values you just aren't asserting for. If you use a dictionary, then it wouldn't be possible that additional key/values snuck in.
| assert response_body == "Planet Pink Star successfully created" | ||
|
|
||
|
|
||
| def test_get_all_planets_with_three_records(client, two_saved_planets): |
There was a problem hiding this comment.
Somewhat confused by the name of this test, since you're testing for two records!
3 participants
Waves 01 and 02