-
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
Zoisite - Nadia and L. - Waves 1 & 2 #17
base: main
Are you sure you want to change the base?
Conversation
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.
Overall, really well done, y'all! This looks great!
# read one planet | ||
# return 400 for invalid id | ||
# return 404 for non existing planet | ||
planets_bp = Blueprint("planets", __name__, url_prefix="/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.
Great job putting together your planet class, your planet list and your planet blueprint!
app/routes.py
Outdated
def handle_planets(): | ||
results = [] | ||
for planet in planets: | ||
results.append(planet.to_dict()) |
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 looks great! Just food for thought, I wonder if this would be a good candidate for a list comprehension... 🤔
app/routes.py
Outdated
if planet.id == planet_id: | ||
return planet | ||
|
||
abort(make_response({"message":f"planet {planet_id} not found"}, 404)) |
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.
The rest of this looks really good!
…tes to include it
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.
Well done, Nadia and L! I left a few comments/things to think about but overall this looks great!
|
||
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.
Nice job placing this above the conditional so we don't repeat code!
id=db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
name=db.Column(db.String, nullable=False) | ||
description=db.Column(db.String, nullable=False) | ||
solar_day=db.Column(db.Float, 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.
Good idea to make these attributes non-nullable! That being said, it can be beneficial to have some nullable attributes. It all boils down to how you want to design your particular class though!
name=planet_data["name"], | ||
description=planet_data["description"], | ||
solar_day=planet_data["solar_day"] | ||
) |
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 not model: | ||
abort(make_response(jsonify({"message":f"{cls.__name__} {model_id} not found"}), 404)) | ||
|
||
return 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.
Great job updating the validate model method to be generic and applicable to any class!
db.session.add(new_planet) | ||
db.session.commit() | ||
|
||
return make_response(jsonify(f"Planet {new_planet.name} created successfully."), 201) |
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.
When you created your Planet class, you made each of the attributes non-nullable, which means that each one needs to be present when an instance of that class is created! With that in mind, it might be a good idea to include some level of error handling when your planet is created in case any of the attributes aren't present!
|
||
db.session.commit() | ||
|
||
return make_response(jsonify(f"Planet {planet.name} updated successfully.")) |
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.
Similarly as before, this method would be a good candidate for error handling as well.
return { | ||
"name": "Walla-Walla", | ||
"description": "A bad place.", | ||
"solar_day": 0.5 |
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 this!
) | ||
|
||
db.session.add_all([nebula, gamora]) | ||
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.
Great job creating a custom fixture to work with multiple planets!
response_body = response.get_json() | ||
|
||
assert response.status_code == 200 | ||
assert response_body == "Planet Walla-Walla updated successfully." |
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.
These tests look great! Well done!
No description provided.