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

Cedar - Rebeca & Lizet #13

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

Cedar - Rebeca & Lizet #13

wants to merge 13 commits into from

Conversation

lizett
Copy link

@lizett lizett commented Oct 18, 2021

No description provided.

Copy link

@beccaelenzil beccaelenzil 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!

app/__init__.py Outdated
Comment on lines 5 to 7
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.

Consider adding a line space to enhance readability

Suggested change
app = Flask(__name__)
from .routes import planets_bp
app.register_blueprint(planets_bp)
app = Flask(__name__)
from .routes import planets_bp
app.register_blueprint(planets_bp)

app/routes.py Outdated
Comment on lines 37 to 42
return {
"id": planet.id,
"name": planet.name,
"description": planet.description,
"moon": planet.moon
}

Choose a reason for hiding this comment

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

Writing an instance method for the Planet class that returns this dictionary with planet data can DRY up your code. You will find that you need to do this work for other route functions too.

Copy link

@kaidamasaki kaidamasaki left a comment

Choose a reason for hiding this comment

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

Well done!

I had one little note around input validation but overall everything looks great!

Comment on lines +9 to +15
def to_json(self):
return {
"id": self.id,
"name": self.name,
"description": self.description,
"moon": self.moon
}

Choose a reason for hiding this comment

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

Nice job adding the helper method Becca recommended. 😃


@planets_bp.route("/<planet_id>", methods=["GET", "PUT", "DELETE"])
def read_planet(planet_id):
planet = Planet.query.get(planet_id)

Choose a reason for hiding this comment

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

One thing to note here, you don't verify the type of planet_id so could potentially get a strange error if someone provides invalid data.

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