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

Extract ES interface from FastAPI routing definition #85

Merged
merged 5 commits into from
Jul 12, 2024
Merged

Conversation

pgulley
Copy link
Member

@pgulley pgulley commented Jul 9, 2024

Creates queries.py, which contains a QueryBuilder and an EsClientWrapper, separating concerns more explicitly. Also removes unused significant/rare term query logic.
(Addressing #71)

Questions:

  • Are there enough modules here now that we want to package the code up in a subdirectory?
  • Can we safely remove all of the config.yml logic? We're no longer using it, in favor of setting things via environment variables.

Copy link
Contributor

@rahulbot rahulbot 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 - I like this restructuring a lot better. I do agree that now that there are multiple modules it'd make sense to have a package they all sit in. I made some suggestions and requests for clarifications or comments in my notes.

api.py Show resolved Hide resolved
api.py Outdated Show resolved Hide resolved
api.py Outdated Show resolved Hide resolved
api.py Show resolved Hide resolved
queries.py Outdated Show resolved Hide resolved
queries.py Outdated Show resolved Hide resolved
queries.py Outdated Show resolved Hide resolved
queries.py Outdated Show resolved Hide resolved
test/__init__.py Show resolved Hide resolved
Copy link
Contributor

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

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

👍🏽


  1. I agree we need to choose whether to continue using config.yml or not. I like the idea of config/settings module that abstract pulling things from system env vars (or .env file) in a type-safe manner. This being a FastAPI app, my vote would be to switch to using pydantic settings instead of building one from scratch around getnev.
  2. On introducing a subdirectory, yeah, an app or src subdirectory would help but the app itself is also very straightforward so I have no issues with not having one.

api.py Outdated Show resolved Hide resolved
queries.py Outdated Show resolved Hide resolved
queries.py Outdated Show resolved Hide resolved
Copy link
Contributor

@philbudne philbudne left a comment

Choose a reason for hiding this comment

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

I'm at the polar opposite from Rahul (who had enough context to make meaningful review comments). Most of the actual ES workings are outside my comfort zone!

api.py Show resolved Hide resolved
@pgulley
Copy link
Member Author

pgulley commented Jul 11, 2024

1. I agree we need to choose whether to continue using `config.yml` or not. I like the idea of `config`/`settings` module that abstract pulling things from system env vars (or `.env` file) in a type-safe manner. This being a FastAPI app, my vote would be to switch to using [`pydantic settings`](https://fastapi.tiangolo.com/advanced/settings/#settings-in-another-module) instead of building one from scratch around `getnev`.

Quick glance at pydantic_settings and I'm impressed- I'll experiment weaving it in now. It should drop-in no problem to the way we're passing in environment variables now, and it would be a one-line change to go back to using a .env file if needed in the future for some reason.

client.py Outdated Show resolved Hide resolved
@pgulley
Copy link
Member Author

pgulley commented Jul 11, 2024

Floating back through here to reiterate that pydantic_settings is wonderful- one of those tools that makes me wonder how I got by without it. Thanks for the tip @kilemensi

@pgulley pgulley removed the request for review from thepsalmist July 12, 2024 16:36
@pgulley pgulley dismissed rahulbot’s stale review July 12, 2024 16:37

Addressed the majority of issues, left comments on those I'm not addressing.

@pgulley pgulley merged commit 8133f35 into main Jul 12, 2024
1 check passed
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