Skip to content

Luqi (Phoenix) & Sunitha(Sphinx) Solar System Wave 01 to Wave 06#5

Open
nelasunitha wants to merge 10 commits intoAda-C22:mainfrom
shiqipaper:main
Open

Luqi (Phoenix) & Sunitha(Sphinx) Solar System Wave 01 to Wave 06#5
nelasunitha wants to merge 10 commits intoAda-C22:mainfrom
shiqipaper:main

Conversation

@nelasunitha
Copy link
Copy Markdown

Please review our code. Thanks!

Copy link
Copy Markdown

@yangashley yangashley 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 here! Your project directory is correctly organized and your model/routes look good.

Comment thread app/routes/__init__.py
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This file should be empty. This logic only needs to be written in app/__init__.py

@nelasunitha nelasunitha changed the title Luqi (Phoenix) & Sunitha(Sphinx) Solar System Wave 01 & Wave 02. Luqi (Phoenix) & Sunitha(Sphinx) Solar System Wave 01 to Wave 06 Oct 31, 2024
@nelasunitha
Copy link
Copy Markdown
Author

Please review our code from wave 01 through wave 06.

Copy link
Copy Markdown

@yangashley yangashley 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 on part 2 of solar system!

name_param = request.args.get("name")
if name_param:
query = query.where(Planet.name.ilike(f"%{name_param}%")).order_by(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.

Remove extra blank line. When we use blank line to break up long chunks of code, we only need one blank line.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

Comment thread tests/conftest.py
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

Comment thread test_fixture.py
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this file just for practice? I'm not clear on what it's testing and it's not inside your test directory.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, We have written this to test for practice.

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