feat: Add reverse geocoding implementation with PostGIS #1
Conversation
emirfabio
changed the title
| """ | ||
| self.conn_string = config.get_connection_string() | ||
|
|
||
| with importlib.resources.path( |
There was a problem hiding this comment.
Interesting! I have never used importlib this way - it's a pretty neat approach.
Pathlib is another option, that I typically use:
from pathlib import Path
self.cities_file = Path(__file__).parent / "data" / "cities_1000_simple.txt.gz"but your approach is probably more immune to project directory restructuring 👍
| RuntimeError: If database query fails | ||
| """ | ||
| # Validate coordinate ranges | ||
| if not -90 <= lat <= 90: |
There was a problem hiding this comment.
Nice to have this validation 👍
| def test_geocoder_initialization(): | ||
| """Test that geocoder can initialize and find its data files.""" | ||
| config = get_test_config() | ||
| geocoder = ReverseGeocoder(config) |
There was a problem hiding this comment.
I think the initialize method needs to be called here potentially, unless we put the call inside the init method, or perhaps a enter method to be used via context manager
There was a problem hiding this comment.
Amazing work!
This should do nicely for our needs 😄
sync / async
One key extra thing to add is async support (which I'm happy to look at if it doesn't interest you 👍).
I wonder if there is any neat way to have a sync and async version of the same class, without having to duplicate the code adding async/await (might be tricky).
One idea could be to use __enter__ for the sync class and __aenter__ to handle a sync and async initialization separately:
# sync
with ReverseGeocoder as reverse_geocode:
...
# async
async with ReverseGeocoder as reverse_geocode:
...Closing the db connection
I think with the current implementation we have a psycopg.connect, but no .close() method called, meaning the connection may remain open?
An advantage of the context manager based approach with ReverseGeocoder as ..., is you can use the __exit__ magic method to do the database closing before the context is closed.
|
Hellooo Sam
I think it'd be nice for the Init function to be both left explicit and have it automatic via context manager. It could run needed checks on the existing data in the db, initialize accordingly and return gracefully.
|
|
That's a very good point! Should we rename the table to something more unique, like |
|
Or probably even better is to do a conditional init depending on if the table already exists! Skip the init if it's already present 😄 What do you think? |
Yup! That's what I was thinking. And also yes, I'll rename the table. In fact, the naming in general is all over the place. We could lean into the NearestCity name everywhere. Does that work for you ? |
Sounds like a plan! |
No description provided.