Skip to content
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 - Jessica and Whitney #29

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

WhitShake
Copy link

No description provided.

Copy link

@apradoada apradoada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall well done! I've left a few comments here or there but all in all this is a very well done project!


def create_app(test_config=None):
app = Flask(__name__)
app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job placing this statement before the conditional so you don't duplicate code!

__tablename__ = 'planets'

id = db.Column(db.Integer, primary_key=True, autoincrement=True)
name = db.Column(db.String, nullable=False)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job making your planets non-nullable! We would definitely want to make sure we include a name when we create a planet!

db.session.add(new_planet)
db.session.commit()

message = f"New planet {new_planet.name} successfully created, biatch!"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve of this message!

if not planet:
abort(make_response({"message": f"Planet {planet_id} was not found, sucker!"}, 404))

return planet

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since validate_planet is a helper method, it is usually a good idea to move it either before or after the routes, or to its own file!

name = request_body["name"],
description = request_body["description"],
mass = request_body["mass"],
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a good candidate for error handling because it is possible a name, description or mass is not included!


return make_response({"message": message}, 200)


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

db.session.delete(planet)
db.session.commit()

message = f"Planet {planet} succesfully deleted, biatch!"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also approve of this message!


message = f"Planet {planet} succesfully updated, biatch!"

return make_response({"message": message}, 200)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be another good candidate for error handling! How could we handle these situations if there was no information provided in the request body?

db.session.add_all(test_planets)
db.session.commit()

return test_planets

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done with the boilerplate code needed for the app and client fixtures and well done writing your custom fixtures for saving planets to the database!

assert response.status_code == 200
assert response_body == {'message': 'Planet <Planet 2> succesfully updated, biatch!'}
assert updated_array == [{'id': 1, 'name': 'test_name_2', 'description': 'test_description_2','mass': '2.000'},
{'id': 2, 'name': 'test_name_1', 'description': 'test_description_1', 'mass': '1.000'}]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done with these tests! They are thoroughly done! And I will always enjoy these messages y'all have created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants