Conversation
…pendencies in pyproject.toml
…s performance by reducing requests.
…for improved resolution
There was a problem hiding this comment.
Pull Request Overview
A release bump to version 0.2.2 that adds CI, optimizes batch resolution, and enhances development dependencies.
- Bumped package version and added dev dependencies
- Implemented unique-value processing in
resolve_batchto reduce API calls - Introduced GitHub Actions CI and updated documentation and Changelog
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_batch.py | Added a real-data integration test (test_batch_real_df) |
| src/georesolver/resolver.py | Overhauled resolve_batch: filtering, unique combos, and DataFrame |
| pyproject.toml | Version bump to 0.2.2 and new [project.optional-dependencies] |
| README.md | Updated project title and added CI badge |
| CHANGELOG.md | Added v0.2.2 section with changes |
| .github/workflows/ci.yml | New CI workflow for tests |
Comments suppressed due to low confidence (3)
src/georesolver/resolver.py:987
- The docstring mentions
use_default_filteras a parameter, but the function signature does not accept it. Either add it to the signature or remove it from the docstring to keep them in sync.
use_default_filter (bool): If True, apply a default filter as fallback.
src/georesolver/resolver.py:1114
- You’ve changed the result DataFrame keys from
place_id/place_uritoid/uri, which breaks the existing DataFrame schema. Consider restoring the original column names or updating downstream consumers and docs.
"id": None, "uri": None, "country_code": None,
src/georesolver/resolver.py:1083
use_default_filteris not defined inresolve_batch. This will raise a NameError. You should add it to the method signature or remove this argument from the call.
use_default_filter=use_default_filter
tests/test_batch.py
Outdated
| assert isinstance(result['longitude'], (int, float)), "Longitude should be numeric" | ||
|
|
||
|
|
||
| def test_batch_real_df(csv_path="tests/data/bautismos_cleaned.csv"): |
There was a problem hiding this comment.
pytest treats any function argument as a fixture, so this test will error on an unknown csv_path fixture. Remove the parameter or convert it into a proper fixture.
| def test_batch_real_df(csv_path="tests/data/bautismos_cleaned.csv"): | |
| import pytest | |
| @pytest.fixture | |
| def csv_path(): | |
| return "tests/data/bautismos_cleaned.csv" | |
| def test_batch_real_df(csv_path): |
| df["country_code"] = "PE" | ||
| df["place_type"] = "city" | ||
|
|
||
| resolver = PlaceResolver([GeoNamesQuery(), WHGQuery()], |
There was a problem hiding this comment.
[nitpick] This test calls live external services, which can lead to flakiness and CI failures if credentials or network are missing. Consider marking it as an integration test or mocking the API responses.
| resolver = PlaceResolver([GeoNamesQuery(), WHGQuery()], | |
| from unittest.mock import MagicMock | |
| # Mock external services | |
| mock_geonames = MagicMock() | |
| mock_geonames.resolve.return_value = {"latitude": 12.34, "longitude": 56.78} | |
| mock_whg = MagicMock() | |
| mock_whg.resolve.return_value = {"latitude": 98.76, "longitude": 54.32} | |
| resolver = PlaceResolver([mock_geonames, mock_whg], |
No description provided.