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

Sharks- Ivana M. and Lauren K. #14

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

Conversation

lkleinert
Copy link

No description provided.

Copy link

@spitsfire spitsfire left a comment

Choose a reason for hiding this comment

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

Part 1! Nicely done! Left a few suggestions here and there, but your logic looks good

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

def to_json(self):

Choose a reason for hiding this comment

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

👍 yay instance methods!

app/routes.py Outdated



planets_bp = Blueprint("planets", __name__, url_prefix = "/planets")

Choose a reason for hiding this comment

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

we only need this assigned once at the top!

Suggested change
planets_bp = Blueprint("planets", __name__, url_prefix = "/planets")

app/routes.py Outdated
Comment on lines 27 to 28
for planet in planets:
planets_response.append(planet.to_json())

Choose a reason for hiding this comment

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

we could also do this with list comprehension:

Suggested change
for planet in planets:
planets_response.append(planet.to_json())
planets_response = [planet.to_json() for planet in planets]

app/routes.py Outdated
for planet in planets:
planets_response.append(planet.to_json())

return jsonify(planets_response)

Choose a reason for hiding this comment

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

don't forget your status codes!

Suggested change
return jsonify(planets_response)
return jsonify(planets_response), 200

Copy link

@spitsfire spitsfire left a comment

Choose a reason for hiding this comment

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

Nicey done, Ivana and Lauren! I didn't find any red flags or any logic issues! Looks good!

"order in solar system": self.order_in_ss
}

def update_planet(self, update_body):

Choose a reason for hiding this comment

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

👍

self.order_in_ss = update_body["order in solar system"]

@classmethod
def create_planet(cls, request_body):

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,14 @@
from flask import abort, make_response

Choose a reason for hiding this comment

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

👍 great job separating this out!

new_planet = cls(
name=request_body['name'],
description=request_body['description'],
order_in_ss=request_body['order in solar system'],

Choose a reason for hiding this comment

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

careful with spaces! while this works syntactically, I don't think it's very common to see keys with spaces like this. Maybe something like order_in_ss might look better

@@ -0,0 +1,55 @@
from app import db

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,41 @@
import pytest

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,64 @@
def test_get_all_planets_with_no_records(client):

Choose a reason for hiding this comment

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

👍

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