-
Notifications
You must be signed in to change notification settings - Fork 69
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
Kunzite - Abby & Aisha #24
base: main
Are you sure you want to change the base?
Conversation
…ote handle_planets(), and moved the GET planet_id route next to handle_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.
Good work with Waves 01-02! Please see specific comments below.
app/routes.py
Outdated
"id": planet.id, | ||
"name": planet.name, | ||
"description": planet.description, | ||
"chemical composition": planet.composition |
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.
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
.
app/routes.py
Outdated
|
||
abort(make_response({"message": f"planet {planet_id} not found"}, 404)) | ||
|
||
def make_dict(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.
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!
app/routes.py
Outdated
|
||
def make_dict(planet): | ||
return dict( | ||
id = 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.
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
)
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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): |
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.
Nice job with this!
bp = Blueprint("planets", __name__, url_prefix="/planets") | ||
# @bp.route("",methods=["GET"]) | ||
|
||
# # helper function |
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.
Again, get in the habit of deleting commented out code.
composition="second") | ||
|
||
db.session.add_all([first_planet, second_planet]) | ||
db.session.commit() |
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.
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")
.
No description provided.