Sharks - Poppy and Danielle #8
Conversation
app/routes.py
Outdated
| return abort(make_response({"message": f"planet {id} is not found"}, 404)) | ||
| # first string is flask blueprint name | ||
| # run in localhost need to add/planets at the end | ||
| solar_bp = Blueprint("", __name__, url_prefix="/planets") |
There was a problem hiding this comment.
The first argument is a string that will be used to identify this Blueprint from the Flask server logs (in the terminal). It's conventional to use a name related to the data being served or the functionality being provided.
solar_bp = Blueprint("planets", __name__, url_prefix="/planets")Also, this is a style point but you can move line 35 higher than your helper method validate_planet() so it's easier to see the Blueprint being instantiated.
app/routes.py
Outdated
|
|
||
|
|
||
| @solar_bp.route("", methods=["GET"]) | ||
| def see_planets(): |
There was a problem hiding this comment.
There's no hard and fast rule about this, but usually for these methods we can use words like 'read' or 'get'. So you could call this method "read_all" or "get_all".
In the future, if you end up splitting your routes up into distinct route files (planet_routes.py and moon_routes.py for example in a routes directory) then you could have method names like get_all() and get_one() without adding the word planet in the function name since all the routes will be related to planet.
app/routes.py
Outdated
| all_planets.append( | ||
| planet.to_json() | ||
| ) | ||
| return jsonify(all_planets), 200 |
There was a problem hiding this comment.
Nice work explicitly adding the 200 status code
app/routes.py
Outdated
| def validate_planet(id): | ||
| try: | ||
| id = int(id) | ||
| except: | ||
| return abort(make_response({"message": f"planet {id} is invalid"}, 400)) | ||
| for planet in planets : | ||
| if planet.id == id: | ||
| return jsonify(planet.to_json()), 200 | ||
| return abort(make_response({"message": f"planet {id} is not found"}, 404)) |
| from app.models.planet import Planet | ||
|
|
||
|
|
||
| def validate_planet(planet_id): |
There was a problem hiding this comment.
Nice work pulling this into a helper to keep your routes short and sweet. In the future, you might need to make 'helper.py' more specific. Think about Task List, you have Goal and Task, you may need helpers for each. If you refactor helper methods into a helper file then you could have goal_helper.py and task_helper.py
| def update(self, req_body): | ||
|
|
||
| self.name = req_body["name"] | ||
| self.description = req_body["description"] | ||
| self.color = req_body["color"] | ||
|
|
||
| @classmethod | ||
| def create(cls, req_body): | ||
| new_planet = cls( | ||
| name=req_body['name'], | ||
| description=req_body['description'], | ||
| color=req_body['color'] | ||
| ) | ||
|
|
||
| return new_planet |
There was a problem hiding this comment.
This comment applies to your create() and update() methods:
How would you add in validation to make sure that all the required fields are sent in the request to your server?
For example, if someone sent a POST request but left off color, then line 27 would throw KeyError that it couldn't find color.
it would be nice to handle the error and return a message so that the client knows their request was invalid and they need to include color. Something to think about for Task List.
|
|
||
| with app.app_context(): | ||
| db.create_all() | ||
| yield app | ||
|
|
||
| with app.app_context(): | ||
| db.drop_all() | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def client(app): | ||
| return app.test_client() | ||
|
|
||
| @pytest.fixture | ||
| def two_saved_planets(app): |
| @@ -0,0 +1,68 @@ | |||
|
|
|||
| def test_get_all_planets_with_no_records(client): | |||
|
|
||
| # GET and POST planets | ||
| @solar_bp.route("", methods=["POST", "GET"]) |
There was a problem hiding this comment.
It's good practice to keep in mind SRP - single responsibility principle for methods here.
Each method should handle its own route so you can split the POST and the GET requests into 2 separate methods. Maybe one called create_planet() for POST and get_planets() for GET
No description provided.