-
Notifications
You must be signed in to change notification settings - Fork 61
Sea Turtles Solar System API Part 1 Tyrah and San #24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
app/routes.py
Outdated
| from flask import Blueprint, jsonify, abort, make_response | ||
|
|
||
| class Planet: | ||
| def __init__(self, id, name, description, has_moon=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for has_moon you can use a boolean value directly as the default. So in this case has_moon=False. We initially used None for the inventory list in swap meet because we wanted to tell whether the call had passed in the inventory list.
app/routes.py
Outdated
| self.description = description | ||
| self.has_moon = has_moon | ||
|
|
||
| def to_json(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great job making this an instance method!
app/routes.py
Outdated
| abort(make_response({"message":f"planet {planet_id} not found"}, 404)) | ||
|
|
||
| @bp.route("", methods=["GET"]) | ||
| def handle_planets(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conventionally you would see the naming of the function be similar to the CRUD operation you are performing. Like get_planets or get_all_planets
app/routes.py
Outdated
| @bp.route("", methods=["GET"]) | ||
| def handle_planets(): | ||
| planet_response = [planet.to_json() for planet in planets] | ||
| return jsonify(planet_response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it's nice to add a 200 status code. Even though it happens by default it adds readability to your code.
return jsonify(planets_result), 200
app/routes.py
Outdated
| return jsonify(planet_response) | ||
|
|
||
| @bp.route("/<planet_id>", methods=["GET"]) | ||
| def get_one_planet(planet_id): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃🏽
| "SQLALCHEMY_DATABASE_URI") | ||
| else: | ||
| app.config["TESTING"] = True | ||
| # app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can go ahead and remove this
| if not test_config: | ||
| app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get('SQLALCHEMY_DATABASE_URI') | ||
| else: | ||
| app.config["TESTING"] = True | ||
| app.config['SQLALCHEMY_TEST_DATABASE_URI'] = os.environ.get('SQLALCHEMY_TEST_DATABASE_URI') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like you all have this twice. Its also on lines 20 - 28. You can remove one of them.
| @@ -0,0 +1,29 @@ | |||
| # from flask import Flask | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this whole file it commented out you can make it an empty __init__.py file.
| name = db.Column(db.String, nullable=False) | ||
| description = db.Column(db.String, nullable=False) | ||
| has_moon = db.Column(db.Boolean, nullable=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you all made this nullable = False
|
|
||
|
|
||
| #planet turn itself into dictionary | ||
| def to_dict(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌🏽
| new_planet = Planet( | ||
| name=request_body["name"], | ||
| description=request_body["description"], | ||
| has_moon=request_body["has_moon"], | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could also make this an instance method in your Planet class.
| has_moon_query = request.args.get("has_moon") | ||
|
|
||
|
|
||
| if name_query: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love these multiple ways to query
| assert response_body == { | ||
| "id": 1, | ||
| "name": "Venus", | ||
| "description": "watr 4evr", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not watr 4evr 😂
| venus_planet = Planet(name="venus", | ||
| description="i luv 2 climb rocks", has_moon=True) | ||
|
|
||
| db.session.add_all([mars_planet, venus_planet]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👩🏽💻
No description provided.