Skip to content

Madeline Bennet, Lula San(Pheonix Class) #11

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

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

Conversation

MBsea21
Copy link

@MBsea21 MBsea21 commented Oct 21, 2024

Wave 1 & wave 2 of solar-system api project.

Copy link

@anselrognlie anselrognlie left a comment

Choose a reason for hiding this comment

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

You're off to a good start. There are some formatting nits, and also be sure to include the error status codes on the expected object (the response rather than the abort call), but otherwise you're right on track.

Copy link

@anselrognlie anselrognlie 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 making the modifications to connect your API to your database, expanding the endpoints, and adding some tests. Everything here should be able to be applied to your task list implementations.

from .routes.planets_routes import planet_bp
import os

def create_app(config=None):

Choose a reason for hiding this comment

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

👍 Thanks for renaming this. This param could be used for scenarios other than testing (the name was left over from the previous curriculum).

app = Flask(__name__)

app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get('SQLALCHEMY_DATABASE_URI')

Choose a reason for hiding this comment

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

👍 Loads the connection string from the environment (provided with our .env during development).

#from .routes.planets_path import planet_bp
def create_app(test_config=None):
from .db import db, migrate
from .models import planet

Choose a reason for hiding this comment

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

Note that once we have the Planet model imported in the routes, and the routes are imported here, we don't technically need to import the planet module here. If you find the fact that VS Code shows this as not being used, it would now be safe to remove.

Comment on lines +11 to +16
return {
"id": self.id,
"name": self.name,
"description": self.description,
"signs of life": self.signs_of_life
}

Choose a reason for hiding this comment

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

Nit: Unindent one level

Comment on lines +4 to +8
class Planet(db.Model):
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
name: Mapped[str]
description: Mapped[str]
signs_of_life: Mapped[bool]

Choose a reason for hiding this comment

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

👍 Nice modifications to associate this model type with our database.

response_body = response.get_json()

assert response.status_code == 404
assert response_body == no_planet_response_body

Choose a reason for hiding this comment

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

👍 We should also check that the response body is the expected message.

Comment on lines 49 to 50
before_post_database_response = client.get("/planets")
before_post_database_response_body =before_post_database_response.get_json()

Choose a reason for hiding this comment

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

This test starts with no fixtures, so these checks are duplicating the get all with no data case. We don't need them here.

def test_post_one_planet(client):
before_post_database_response = client.get("/planets")
before_post_database_response_body =before_post_database_response.get_json()
response=client.post("/planets",json={

Choose a reason for hiding this comment

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

Nit: Spaces around assignment and after comma

    response = client.post("/planets", json={

Comment on lines 58 to 59
print("actual response", response_body)
print("expected response", planet_1_response_body)

Choose a reason for hiding this comment

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

Nit: Remove debug output.

Comment on lines 60 to 61
assert before_post_database_response.status_code == 200
assert before_post_database_response_body == []

Choose a reason for hiding this comment

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

These checks aren't need, as above.

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