Skip to content

Cedar - Rebeca & Lizet#13

Open
lizett wants to merge 13 commits intoAda-C16:mainfrom
lizett:main
Open

Cedar - Rebeca & Lizet#13
lizett wants to merge 13 commits intoAda-C16:mainfrom
lizett:main

Conversation

@lizett
Copy link
Copy Markdown

@lizett lizett commented Oct 18, 2021

No description provided.

Copy link
Copy Markdown

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

Comment thread app/__init__.py
Comment on lines 5 to +7
app = Flask(__name__)
from .routes import planets_bp
app.register_blueprint(planets_bp)
Copy link
Copy Markdown

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)

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

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
Copy Markdown

@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 thread app/models/planet.py
Comment on lines +9 to +15
def to_json(self):
return {
"id": self.id,
"name": self.name,
"description": self.description,
"moon": self.moon
} No newline at end of file
Copy link
Copy Markdown

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. 😃

Comment thread app/routes.py

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

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