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

Snow Leopards - Cristal & Francesca #39

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

Conversation

FrancescaFr
Copy link

No description provided.

Copy link

@anselrognlie anselrognlie left a comment

Choose a reason for hiding this comment

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

👍 Keep it up for Task List API!

Be sure to follow any final refactors shown in Learn (e.g. splitting POST and GET ALL into separate routes), and refer back to the readings for anything else that you didn't have time to finish up in activity time while working on Task List.


db = SQLAlchemy()
migrate = Migrate()
load_dotenv()

def create_app(test_config=None):

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,23 @@
from app import db

class Planet(db.Model):

Choose a reason for hiding this comment

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

👍


planet_bp = Blueprint("planets", __name__, url_prefix="/planets")

def validate_planet(id):

Choose a reason for hiding this comment

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

👀 Consider refactoring this to receive the class type as a parameter so that our validate helper could work for more than one type. This will be helpful in Task List when adding the Goal model.


return planet

@planet_bp.route("", methods=["GET", "POST"])

Choose a reason for hiding this comment

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

Be sure to refer to the second part of creating the get all endpoint where we avoid using multiple verbs in a single endpoint.

The multiple verb approach is possible, but generally we want to avoid that (single responsibility principle).

name_query = request.args.get("name")
flag_query = request.args.get("flag")

planet_query = Planet.query

Choose a reason for hiding this comment

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

👍 Nice job applying multiple filters.

def get_one_planet(id):
validate_planet(id)
planet = Planet.query.get(id)
return {"id": planet.id,

Choose a reason for hiding this comment

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

👀 Remember to use your .to_dict() here.

@@ -0,0 +1,28 @@
"""removed flag

Choose a reason for hiding this comment

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

Nice to see that you went through a few migrations. Keep in mind that once you've gotten your "final" table layour squared away that it can be helpful to clear out all th old migrations, recreate the empty database, and get 1 nice, clean migration. Not something we'd want to do after having real data in a production database, but it's nice to start without migration clutter.

return app.test_client()

@pytest.fixture
def one_planet(app):

Choose a reason for hiding this comment

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

👍 Nice single-planet fixture. And great job returning the value so that it can be used in a test directly!

flag=request_body["flag"])
db.session.add(new_planet)
db.session.commit()
return make_response(f"planet {new_planet.name} successfully created!", 201)

Choose a reason for hiding this comment

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

👀 Make sure to jsonify even plain string responses, otherwise flask will return an HTML response rather than JSON.

@@ -1,2 +1,53 @@
from flask import Blueprint
from flask import Blueprint, jsonify, abort, make_response, request

Choose a reason for hiding this comment

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

👀 Make sure to review the readings for read, update, and delete (part 4) to make sure you have an idea for how to approach those.

Copy link
Author

@FrancescaFr FrancescaFr Nov 9, 2022

Choose a reason for hiding this comment

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

Oh no! we somehow didn't push wave 4 of solar system...looks like we picked up on an older version to complete the rest of the waves... Should we update the pull request or leave it as is?

Copy link
Author

@FrancescaFr FrancescaFr Nov 9, 2022

Choose a reason for hiding this comment

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

There are the additions in this commit - then I don't see how they disappeared: Commit: Wave 4 complete

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