Skip to content

Phoenix + Sphinx - Aksana + Brianna #12

Open
bri-root wants to merge 15 commits intoAda-C22:mainfrom
Kseny-a:main
Open

Phoenix + Sphinx - Aksana + Brianna #12
bri-root wants to merge 15 commits intoAda-C22:mainfrom
Kseny-a:main

Conversation

@bri-root
Copy link

No description provided.

Copy link

@mikellewade mikellewade left a comment

Choose a reason for hiding this comment

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

Great work! ✅

migrate.init_app(app, db)

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.

Comment on lines +4 to +8
class Planet(db.Model):
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
name: Mapped[str]
description: Mapped[str]
num_moons: Mapped[int]

Choose a reason for hiding this comment

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

We could have some methods on this class that could make our business logic in our routes more clean and in line with SRP

Comment on lines +17 to +23
# def to_dict(self):
# return dict(
# id = self.id,
# name = self.name,
# description = self.description,
# num_moons = self.num_moons,
# )

Choose a reason for hiding this comment

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

Like this here could be added to our Planet class.

Comment on lines +18 to +23
response = {
"id": new_planet.id,
"name": new_planet.name,
"description": new_planet.description,
"num_moons": new_planet.num_moons
}

Choose a reason for hiding this comment

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

This is a place where you could call that method, like so new_planet.to_dict().

"num_moons": planet.num_moons
}
)
return planets_response

Choose a reason for hiding this comment

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

Comment on lines +61 to +66
return {
"id": planet.id,
"name": planet.name,
"description": planet.description,
"num_moons": planet.num_moons,
}

Choose a reason for hiding this comment

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

Another place where you could call to_dict.

Comment on lines +73 to +76
planet.name = request_body["name"]
planet.description = request_body["description"]
planet.num_moons = request_body["num_moons"]
db.session.commit()

Choose a reason for hiding this comment

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

Could this be its own helper function perhaps?

Comment on lines +88 to +102
def validate_planet(planet_id):
try:
planet_id =int(planet_id)
except:
response = {"message": f"Planet {planet_id} invalid."}
abort(make_response(response, 400))

query = db.select(Planet).where(Planet.id==planet_id)
planet = db.session.scalar(query)

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

return planet

Choose a reason for hiding this comment

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

Now that we've learned a little more about refactoring, where could this function live?

Comment on lines +35 to +42
@pytest.fixture
def two_saved_planets(app):
# Arrange
planet_1 = Planet(name="Mercury", description="first planet", num_moons=0)
planet_2 = Planet(name="Venus", description="second planet", num_moons=0)

db.session.add_all([planet_1, planet_2])
db.session.commit()

Choose a reason for hiding this comment

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

⭐️

"name": "Earth",
"description": "Blue planet",
"num_moons": 1
}

Choose a reason for hiding this comment

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

Nice work on these tests! How could we modify them to make sure that our changes (additions and deletions) are being persisted to our database?

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