Sphinx + Phoenix - Sarah K., Charday N., & Priyanka K. #66
Conversation
…efined Planet class and hardcoded a list of instances, and created an endpoint to get all existing planets. Co-authored-by: Charday Neal <[email protected]>
app/routes.py
Outdated
There was a problem hiding this comment.
Even though we only have one model, planets, we'll still want a directory called "routes" and this file would nest under that directory and should be renamed planet_routes.py
Consider making this change before starting on Wave 3!
app/__init__.py
Outdated
| @@ -1,7 +1,10 @@ | |||
| from flask import Flask | |||
| from .routes import planets_bp | |||
There was a problem hiding this comment.
See comment in routes.py first, how would this import statement need to change if your route file was named planet_routes.py and was in a "routes" directory?
app/planets.py
Outdated
There was a problem hiding this comment.
Even though we only have one model, planets, we'll still want a directory called "models" and this file would nest under that directory.
Soon you'll be working with APIs that have multiple models and each model will have specific routes so organizing your project accordingly makes it easier to navigate.
Consider making this change before starting on Wave 3!
Also, since the class name is singular, I'd expect that the file name would match the class name so planets.py would be called planet.py
requirements.txt
Outdated
There was a problem hiding this comment.
Did dependencies get updated? Do we need to be tracking these changes in the requirements.txt? Maybe i missed something since I wasn't in the round table, but I wouldn't expect needing to make changes to this file.
There was a problem hiding this comment.
We both had some issues installing Flask, but we didn't intend to make any changes to the requirements.txt (and also missed that this was included in the commit). We'll revert the changes with the next push.
…ase and Flask. Defined Book Model. Imported model explicitly. Completed initial setup of the database on the Flask end. Generated and applied migration files adding Book model.
skoch-neat
changed the title
… to use absolute imports. Reorganized import statements, grouped by source and sorted alphabetically within groups. Removed old, unused code. Ensured consistency in the use of single and double quotes in strings.
PEP8 Cleanup
…and delete a planet. Co-authored-by: Pri <[email protected]> Co-authored-by: Charday Neal <[email protected]>
… Add seed data for database.
"Add wave 6 fixtures and pytest to validate get one planet-found, get one planet-not found, get all planet-empty-db, get all planet- db has 2 entries, post one planet. Add two_saved_planets to database."
…y. Added helper functions to get query parameters from the client request, filter the query based on the client parameters, sort the response body by a client-supplied parameter, and supporting validation for data processed.
constants.py
Outdated
| if params[ID]: | ||
| query = query.where(Planet.id == validate_cast_type(params[ID], int, ID)) |
There was a problem hiding this comment.
If I'm understanding correctly, this handles a scenario where someone makes a GET requets to /planets and could pass along query param like ?id=1. Then you'd "filter", query for record with id 1. Is that right?
If that's the case, we duplicate functionality because your GET /planets/id already achieves this and also follows convention. I'd prefer getting rid of this filtering logic since it's duplicating what you already have.
There was a problem hiding this comment.
I think maybe renaming the function to validate_and_cast_value would be a little more descriptive because your function does more than validating. If all goes according to plan, it will return a new type for the value passed in.
tests/test_planet_routes.py
Outdated
tests/conftest.py
Outdated
No description provided.