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

Pine: Melinda and Mac #12

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

Pine: Melinda and Mac #12

wants to merge 22 commits into from

Conversation

mac6551
Copy link

@mac6551 mac6551 commented Oct 18, 2021

No description provided.

Copy link

@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.

app/routes.py Outdated

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

Choose a reason for hiding this comment

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

Neat solution!

app/routes.py Outdated
planet_id = int(planet_id)
for planet in planets:
if planet.id == planet_id:
return vars(planet)

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

@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!

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'

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")

description = db.Column(db.String)
moons = db.Column(db.String)

def to_dict(self):

Choose a reason for hiding this comment

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

Great helper

Deletes specified planet entry and return success message for DELETE request."""

planet_id = int(planet_id)
planet = Planet.query.get_or_404(planet_id)

Choose a reason for hiding this comment

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

Neat!

@@ -0,0 +1,6 @@
def test_handle_planets_returns_200_and_empty_array(client):

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