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

Sphinx-Weixi_He, Phoenix- Madina Dzhetegenova #67

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions app/__init__.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
from flask import Flask
from .routes.routes import solar_system_bp


def create_app(test_config=None):
def create_app():
app = Flask(__name__)

app.register_blueprint(solar_system_bp)

return app
Empty file added app/models/__init__.py
Empty file.
19 changes: 19 additions & 0 deletions app/models/planet.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@

class Planet:
def __init__(self, id, name, description, flag= bool):
Copy link
Contributor

Choose a reason for hiding this comment

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

👀 The expression for the default would generally be a value of the desired type, rather than the type name itself.

    def __init__(self, id, name, description, flag=False):

As written, if we left of the flag value, then self.flag would be literally set to the bool type, not a boolean value (True/False).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not immediately obvious that the attrbiute called flag is intended to store a boolean. Consider a name like has_flag. Variables that start with words like is_ or has_ (depending on what makes grammatical sense) make it more clear that this is intended to hold a yes/no (boolean) value.

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


planets = [
Planet(1, "Mercury", "The smallest planet in our solar system.", False),
Planet(2, "Venus", "The hottest planet in the solar system.", False),
Planet(3, "Earth", "The only planet known to support life.", True ),
Planet(4, "Mars", "Known as the Red Planet.", True ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: be careful of extra spaces.

    Planet(3, "Earth", "The only planet known to support life.", True), 
    Planet(4, "Mars", "Known as the Red Planet.", True), 

Planet(5, "Jupiter", "The largest planet in our solar system.", False),
Planet(6, "Saturn", "Famous for its ring system.", False),
Planet(7, "Uranus", "An ice giant with a unique tilt.", False),
Planet(8, "Neptune", "The farthest planet from the Sun.", False)
]
2 changes: 0 additions & 2 deletions app/routes.py

This file was deleted.

Empty file added app/routes/__init__.py
Empty file.
42 changes: 42 additions & 0 deletions app/routes/routes.py
Copy link
Contributor

Choose a reason for hiding this comment

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

When moving the routes file into a folder (theoretically, to group multiple routes files together as we add additional resources), prefer to include the resource name as part of the file name. So here, something like planets_routes.py or planet_routes.py.

Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
from flask import Blueprint, abort, make_response
from ..models.planet import planets
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice relative import.


solar_system_bp = Blueprint("solar_system_bp", __name__, url_prefix="/planets")
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer to use the resource name (rather than the project name) as part of the blueprint name. So here, planets_bp, since this blueprint is related to the planets resources.


@solar_system_bp.get("")
def get_all_planets():
results_list = []
for planet in planets:
results_list.append(dict(
id=planet.id,
name=planet.name,
description=planet.description,
flag=planet.flag
))
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though this is using the dict() style and later (in get_one_planet) we use the {} style, notice how both are building the same dictionary structure from a planet record. Maybe we could make a helper function/method to DRY up our code.


return results_list

@solar_system_bp.get("/<planet_id>")
def get_one_planet(planet_id):
planet = validate_planet(planet_id)

return{
"id":planet.id,
"name":planet.name,
"description":planet.description,
"flag":planet.flag
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: for dictionary literals, prefer spaces after the :

    return {
        "id": planet.id,
        "name": planet.name,
        "description": planet.description,
        "flag": planet.flag
    }


def validate_planet(planet_id):
try:
planet_id = int(planet_id)
except:
response = {"message": f"planet {planet_id} invalid"}
abort(make_response(response, 400))

for planet in planets:
if planet.id == planet_id:
return planet

response = {"message": f"planet {planet_id} not found"}
abort(make_response(response, 404))