Skip to content

Pine: Melinda and Mac#12

Open
mac6551 wants to merge 22 commits intoAda-C16:mainfrom
mac6551:main
Open

Pine: Melinda and Mac#12
mac6551 wants to merge 22 commits intoAda-C16:mainfrom
mac6551:main

Conversation

@mac6551
Copy link
Copy Markdown

@mac6551 mac6551 commented Oct 18, 2021

No description provided.

Copy link
Copy Markdown

@jbieniosek jbieniosek left a comment

Choose a reason for hiding this comment

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

Great work on this project! This project is green.

Comment thread app/routes.py Outdated

@planets_bp.route("", methods=["GET"])
def get_all_planets():
planets_response = [vars(planet) for planet in planets]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Neat solution!

Comment thread app/routes.py Outdated
planet_id = int(planet_id)
for planet in planets:
if planet.id == planet_id:
return vars(planet) No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I recommend adding ‘jsonify’ to the return even in the case where the Flask default behavior is doing the same thing. Using ‘jsonify’ adds a layer of error protection and also makes it clear that json is the intended behavior.

Suggested change
return vars(planet)
return jsonify(vars(planet))

Copy link
Copy Markdown

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Nice work Mac & Melinda, you hit the learning goals here. Well done!

Comment thread app/__init__.py Outdated
if not test_config:
app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
app.config['SQLALCHEMY_DATABASE_URI'] = \
'postgresql+psycopg2://postgres:postgres@localhost:5432/solar_system_development'
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just a note: In future projects we can use an environmental variable rather than hard-coding the connection string here, like you do below with os.environ.get( "SQLALCHEMY_TEST_DATABASE_URI")

Comment thread app/models/planet.py
description = db.Column(db.String)
moons = db.Column(db.String)

def to_dict(self):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great helper

Comment thread app/routes.py
Deletes specified planet entry and return success message for DELETE request."""

planet_id = int(planet_id)
planet = Planet.query.get_or_404(planet_id)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Neat!

Comment thread tests/test_routes.py
@@ -0,0 +1,6 @@
def test_handle_planets_returns_200_and_empty_array(client):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Awesome a test!

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.

4 participants