Skip to content

C17 Sea turtles, Celina Barron, Shelby Faulconer, and Tiffini Hyatt#13

Open
pickled-bot wants to merge 26 commits intoada-c17:mainfrom
tiffinihyatt:main
Open

C17 Sea turtles, Celina Barron, Shelby Faulconer, and Tiffini Hyatt#13
pickled-bot wants to merge 26 commits intoada-c17:mainfrom
tiffinihyatt:main

Conversation

@pickled-bot
Copy link
Copy Markdown

Waves 1 and 2

Copy link
Copy Markdown

@kelsey-steven-ada kelsey-steven-ada 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 y'all! I've left a few comments, please reach out if you have any questions on the feedback 🙂

Comment thread app/routes.py Outdated
Comment on lines +44 to +45
}
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If the opening brackets are stacked together, I'd recommend doing the same with the closing brackets to be consistent.

Comment thread app/routes.py Outdated
Comment on lines +39 to +45
planet_list.append({
"id" : planet.id,
"name" : planet.name,
"description" : planet.description,
"distance from sun" : planet.dist_from_sun
}
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

To reduce repetition, we could reuse Planet's to_dict function here:

planet_list.append(planet.to_dict())

Comment thread app/routes.py
return planet.to_dict()


def validate_planet(planet_id):
Copy link
Copy Markdown

@kelsey-steven-ada kelsey-steven-ada Apr 27, 2022

Choose a reason for hiding this comment

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

I'd consider renaming this function. To me, validate_planet sounds like it should return a true/false based on if the planet passed as a parameter is valid. What name might better describe what the function is doing?

Comment thread app/routes.py Outdated
Comment on lines +21 to +31
planets = [
Planet(1, "Mercury", "rocky", 1),
Planet(2, "Venus", "rocky", 2),
Planet(3, "Earth", "water", 3),
Planet(4, "Mars", "red", 4),
Planet(5, "Jupiter", "big", 5),
Planet(6, "Saturn", "rings", 6),
Planet(7, "Uranus", "butt", 7),
Planet(8, "Neptune", "ice", 8),
Planet(9, "Pluto", "dwarf", 9)
]
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 see what you did there 😆

Comment thread app/routes.py Outdated
if planet.id == planet_id:
return planet

abort(make_response({"message": f"planet {planet_id} not found"}, 404))
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 error handling in the helper function!

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