Skip to content

Sphinx-Denise and Olga #16

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 8 commits into
base: main
Choose a base branch
from
Open

Sphinx-Denise and Olga #16

wants to merge 8 commits into from

Conversation

lioliadoc
Copy link

No description provided.

Copy link

@yangashley yangashley 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! The project is correctly organized and your model and routes look good. Let me know if you have questions about my comments.

self.id = id
self.name = name
self.description = description

Choose a reason for hiding this comment

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

How would you write the to_dict instance method so you can take an instance of Planet and turn it into a dictionary that will be returned as a response to a client's request?

Comment on lines 25 to 31
for planet in planets:
if planet.id == int(planet_id):
return {
"id": planet.id,
"name": planet.name,
"description": planet.description,
}

Choose a reason for hiding this comment

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

After your model has been validated, we don't need to loop through all the planets again to see if the id matches. Have a look at the validate_planet function and see that you already check to see if the id matches on line 40-42.

Suggested change
for planet in planets:
if planet.id == int(planet_id):
return {
"id": planet.id,
"name": planet.name,
"description": planet.description,
}
return {
"id": planet.id,
"name": planet.name,
"description": planet.description,
}

Comment on lines 47 to 62
# @planets_bp.post("")
# def create_planet():
# request_body = request.get_json()
# title = request_body["title"]
# description = request_body["description"]

# new_book = Planet(title=title, description=description)
# db.session.add(new_planet)
# db.session.commit()

# response = {
# "id": new_planet.id,
# "title": new_planet.name,
# "description": new_planet.description,
# }
# return response, 201

Choose a reason for hiding this comment

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

We'll have a look at how the model needs to be updated in order for the POST route to work!

Copy link

@yangashley yangashley 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 on part 2 of solar-system!

Comment on lines +33 to +38
{
"id": planet.id,
"name": planet.name,
"description": planet.description,
"diameter": planet.diameter
}

Choose a reason for hiding this comment

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

Prefer your Planet model has a to_dict method so that it can be called in your different routes instead repeating code. It would also help your routes be more concise.

Comment on lines +54 to +60
response = {

"id": new_planet.id,
"name": new_planet.name,
"description": new_planet.description,
"diameter": new_planet.diameter
}

Choose a reason for hiding this comment

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

Could call to_dict here too

Comment on lines +68 to +74
return {

"id": planet.id,
"name": planet.name,
"description": planet.description,
"diameter": planet.diameter
}

Choose a reason for hiding this comment

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

Could call to_dict on planet here instead so creating this literal dictionary isn't happening three different times in your route file.

Choose a reason for hiding this comment

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

👍

Choose a reason for hiding this comment

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

👍

test_fixtures.py Outdated

Choose a reason for hiding this comment

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

Is this file just for practice? I'm not clear on what it's testing and it's not inside your test directory.

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