-
Notifications
You must be signed in to change notification settings - Fork 4
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
FastAPI app #27
FastAPI app #27
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing! I added a review only because I was checking this out to learn. Feel free to dismiss all the comments and carry on!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some first comments from setting up locally
ets: Annotated[list[float], Query()] | str | None = None, | ||
startEts: float | None = None, | ||
exposureDuration: float | None = None, | ||
numOfExposures: int | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe Im missing something, but isn't it better to make a new endpoint rather than adding to the existing one? Im not 100% sure what would be the better practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so too, but I've tried that and found that FastAPI doesn't seem to support having duplicate endpoints with varying query params. Seems like the general consensus was to have one endpoint with either optional params and data validation (example) or create two models (one ets
only, other with startEts
etc.) where the input data can be validated that way (example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach isn't bad 🤔 thinking about it over the weekend. I think I prefer it over another endpoint with a different name.
These files will standup a local instance of SpiceQL as a FastAPI app. There is a
README
to follow as well. The endpoints mirror the existing endpoints of the SpiceQL-AWS instance.