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

DM-43939: Create a bootcamp demo app showing basic path operations #2

Merged
merged 10 commits into from
Apr 29, 2024

Conversation

jonathansick
Copy link
Member

@jonathansick jonathansick commented Apr 17, 2024

Covers path parameters, query parameters, response models, and post requests with request models. Cover basic logging with Safir's logger_dependency.

In the next PR, #3, we add to this with a realistic app to show the sqr-072.lsst.io-type architecture.

For this "external" router, we'll use it to demonstrate basic path
operations, and keeping the models co-located with the path operation
functions makes it a little easier to demonstrate. By getting rid of the
models module, we also make space for a better architecture for
separating internal and API models.
I think this was a bug in our template that'll be fixed in
lsst/templates#253
Cover path parameters, query parameters, response models, and post
requests with request models. Cover basic logging with Safir's
logger_dependency.
We'll need to modify the template to deal with hyphenated app names.
Copy link
Member

@rra rra left a comment

Choose a reason for hiding this comment

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

This looks great. Really easy to read and a very natural flow. Great work!

@@ -27,7 +27,7 @@ class Config(BaseSettings):
)

model_config = SettingsConfigDict(
env_prefix="FASTAPI-BOOTCAMP_", case_sensitive=False
env_prefix="FASTAPI_BOOTCAMP_", case_sensitive=False
Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines 81 to 87
# =============================================================================
# Lesson 1: A simple GET endpoint.


@external_router.get("/hello", summary="Get a friendly greeting.")
async def get_greeting() -> str:
return "Hello, SQuaRE Services Bootcamp!"
Copy link
Member

Choose a reason for hiding this comment

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

I think I agree with keeping this simple, but it could be a bit surprising that this handler still returns JSON, so the response will have quote marks around it. The simplest response from an HTTP perspective would require a bit more complexity in FastAPI to specify response_class=PlainTextResponse. Maybe that's worth mentioning in a comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't think about that, but mentioning this will definitely clarify things.

Rather than mixing JSON and query parameters in the same example, let's
do the JSON response first, and then add in the query parameter next.
@jonathansick jonathansick merged commit b10c61f into main Apr 29, 2024
3 checks passed
@jonathansick jonathansick deleted the tickets/DM-43939 branch April 29, 2024 20:03
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.

2 participants