-
Notifications
You must be signed in to change notification settings - Fork 0
Add geocoding functionality and endpoint to the backend #5
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
base: develop
Are you sure you want to change the base?
Conversation
| black services/backend/api services/backend/tests --check | ||
| - name: Test with pytest | ||
| env: | ||
| GEOCODER_API_KEY: ${{ secrets.GEOCODER_API_KEY }} |
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.
FYI, this is how you add secrets to Github and then expose them in CI: https://docs.github.com/en/actions/security-guides/encrypted-secrets
| volumes: | ||
| - ./services/backend/api:/app/api | ||
| env_file: | ||
| - .env |
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.
This is a simple way to specify that you want docker to load environment vars from a file with a key=value format.
|
|
||
|
|
||
| @app.get("/location/{location_query}", response_model=Location) | ||
| def get_geocoded_location(location_query: str): |
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.
We can't make this an async function because it ultimately relies on requests which currently doesn't support async. There are async http libs but I don't care enough to learn them.
| raise ValueError(f"Unable to parse {location_string}. Expected format 'lat+lon'") | ||
|
|
||
|
|
||
| @lru_cache(maxsize=256) |
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.
Adding caching seemed like a handy little trick. Suppose if we really wanted to get fancy we could add a full DB cache that would persist between runs.
|
|
||
| @property | ||
| def hash(self) -> str: | ||
| return hashlib.md5(self.hashable_name).hexdigest() |
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.
Honestly not sure why I wrote this. I think I had some idea about matching values by hash lookup, but now I can't really think of a way that would be useful rn...
This PR is a first stab at adding a location geocoding service to the backend using the Positionstack API.
Not sure if this is too convoluted, but the flow I imagines was:
An obvious question is: why does the lat+lon need to be passed back and forth, why not just do the geocoding and forecast fetching in one request? My thinking is making this multi-step: 1. allows the frontend to use its own state to determine if a new forecast is necessary (user might have reentered the same location as a previous one), and 2) allows us to use the location service for other stuff, like auto-populating search suggestions.
idk I'm mostly just noodling around, very open to redesigning this...
Oh the geocoder API requires a key stored as an env var called
GEOCODER_API_KEY. Docker looks for these creds in a.envfile stored in the root directory. Happy to send you mine, or you can sign up for your own free key.I added the key as a GitHub secret. Let's see if the CI tests work...