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

Zoisite - Jessica and Whitney - Waves 1 & 2 #19

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

Conversation

WhitShake
Copy link

No description provided.

Copy link

@apradoada apradoada left a comment

Choose a reason for hiding this comment

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

Overall this looks really great! Well done, y'all!

@@ -4,4 +4,7 @@
def create_app(test_config=None):
app = Flask(__name__)

from .routes import planets_bp
app.register_blueprint(planets_bp)

Choose a reason for hiding this comment

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

This looks great!

app/routes.py Outdated
Planet(6, "Saturn", "Known for its prominent rings made of ice and dust", 568),
Planet(7, "Uranus", "Blue-green planet with a tilted axis of rotation", 86.8),
Planet(8, "Neptune", "Blue planet with a windy atmosphere and many storms", 102),
]

Choose a reason for hiding this comment

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

Great job setting up your planet class and planet list!

app/routes.py Outdated
for planet in planet_list:
planets_response.append(planet.to_dict())

return jsonify(planets_response), 200

Choose a reason for hiding this comment

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

This looks good, but I wonder if it would be a good candidate for a list comprehension... 🤔

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

abort(make_response({"message": f"Planet {id} is not found!"}, 404))

Choose a reason for hiding this comment

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

The rest of this looks good too! Well done!

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