feat: canonicalize addresses#108
Conversation
📝 WalkthroughWalkthroughThis PR extends the enrichment pipeline to extract and preserve structured address components from Google Places API responses, adding new model fields, parsing logic, country inference from enrichment cache, and comprehensive test coverage for ambiguous guide location inference. ChangesAddress Components Enrichment
Sequence DiagramsequenceDiagram
participant PlacesAPI as Google Places API
participant extract as extract_api_address_components
participant normalize as normalize_enrichment_match
participant cache as enrichment_cache
participant infer as infer_country_from_enrichment_cache
participant guide as normalize_guide
participant model as EnrichmentPlace
PlacesAPI->>extract: addressComponents list
extract->>normalize: {country_name, country_code, admin_area, locality, postal_code}
normalize->>model: formatted_address_en, address_* fields
cache->>infer: scan publishable entries
infer->>guide: most common (country_name, country_code)
guide->>model: fallback country when title inference fails
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| [dependency-groups] | ||
| dev = [ | ||
| "mypy>=1.18.2,<2.0.0", | ||
| "pytest>=9.0.3", |
There was a problem hiding this comment.
I wasn't able to find pytest when I used uv so I added this, apologies if not needed!
| @@ -1,14 +1,14 @@ | |||
| version = 1 | |||
| revision = 1 | |||
| revision = 3 | |||
There was a problem hiding this comment.
Please let me know if I should remove this from the PR , thanks!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pyproject.toml (1)
19-19: ⚡ Quick winConstrain
pytestto the intended major range.
pytest>=9.0.3is unbounded. The latest stable release ispytest 9.0.3, and the pytest project reserves breaking-deprecation removals for major releases—pytest 10 is already documented with breaking removals/deprecations. To prevent surprise CI failures, match the neighboring dev-dependency style by using an upper bound likepytest>=9.0.3,<10.0.0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` at line 19, The pytest dev-dependency is unbounded ("pytest>=9.0.3"), which can allow pytest 10 with breaking changes; update the dependency string to constrain the major range (for example change "pytest>=9.0.3" to "pytest>=9.0.3,<10.0.0") so CI isn't surprised by pytest 10; locate the dependency entry containing pytest>=9.0.3 in pyproject.toml and add the <10.0.0 upper bound.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build_data.py`:
- Around line 6461-6466: The current block only copies previous_place.address_*
fields when address_country_name is missing, which loses valid refreshed
subcomponents; update the logic to preserve each address component independently
by checking each field separately (for example, for address_country_name,
address_country_code, address_admin_area, address_locality, address_postal_code)
and assign refreshed_place.<field> = previous_place.<field> only when
refreshed_place.<field> is empty/falsey and previous_place.<field> is present,
using individual if checks for refreshed_place and previous_place on each named
field.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 19: The pytest dev-dependency is unbounded ("pytest>=9.0.3"), which can
allow pytest 10 with breaking changes; update the dependency string to constrain
the major range (for example change "pytest>=9.0.3" to "pytest>=9.0.3,<10.0.0")
so CI isn't surprised by pytest 10; locate the dependency entry containing
pytest>=9.0.3 in pyproject.toml and add the <10.0.0 upper bound.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cda3bb0f-2f01-42fb-9447-a44e5b71f691
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
pyproject.tomlscripts/build_data.pyscripts/pipeline_models.pytests/python/test_build_data.py
| if not refreshed_place.address_country_name and previous_place.address_country_name: | ||
| refreshed_place.address_country_name = previous_place.address_country_name | ||
| refreshed_place.address_country_code = previous_place.address_country_code | ||
| refreshed_place.address_admin_area = previous_place.address_admin_area | ||
| refreshed_place.address_locality = previous_place.address_locality | ||
| refreshed_place.address_postal_code = previous_place.address_postal_code |
There was a problem hiding this comment.
Preserve address component fields independently.
On Line 6461, preservation of address_country_code, address_admin_area, address_locality, and address_postal_code is incorrectly gated by address_country_name. If refreshed data keeps country name but drops other components, prior valid data is lost.
💡 Proposed fix
- if not refreshed_place.address_country_name and previous_place.address_country_name:
- refreshed_place.address_country_name = previous_place.address_country_name
- refreshed_place.address_country_code = previous_place.address_country_code
- refreshed_place.address_admin_area = previous_place.address_admin_area
- refreshed_place.address_locality = previous_place.address_locality
- refreshed_place.address_postal_code = previous_place.address_postal_code
+ if not refreshed_place.address_country_name and previous_place.address_country_name:
+ refreshed_place.address_country_name = previous_place.address_country_name
+ if not refreshed_place.address_country_code and previous_place.address_country_code:
+ refreshed_place.address_country_code = previous_place.address_country_code
+ if not refreshed_place.address_admin_area and previous_place.address_admin_area:
+ refreshed_place.address_admin_area = previous_place.address_admin_area
+ if not refreshed_place.address_locality and previous_place.address_locality:
+ refreshed_place.address_locality = previous_place.address_locality
+ if not refreshed_place.address_postal_code and previous_place.address_postal_code:
+ refreshed_place.address_postal_code = previous_place.address_postal_code📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not refreshed_place.address_country_name and previous_place.address_country_name: | |
| refreshed_place.address_country_name = previous_place.address_country_name | |
| refreshed_place.address_country_code = previous_place.address_country_code | |
| refreshed_place.address_admin_area = previous_place.address_admin_area | |
| refreshed_place.address_locality = previous_place.address_locality | |
| refreshed_place.address_postal_code = previous_place.address_postal_code | |
| if not refreshed_place.address_country_name and previous_place.address_country_name: | |
| refreshed_place.address_country_name = previous_place.address_country_name | |
| if not refreshed_place.address_country_code and previous_place.address_country_code: | |
| refreshed_place.address_country_code = previous_place.address_country_code | |
| if not refreshed_place.address_admin_area and previous_place.address_admin_area: | |
| refreshed_place.address_admin_area = previous_place.address_admin_area | |
| if not refreshed_place.address_locality and previous_place.address_locality: | |
| refreshed_place.address_locality = previous_place.address_locality | |
| if not refreshed_place.address_postal_code and previous_place.address_postal_code: | |
| refreshed_place.address_postal_code = previous_place.address_postal_code |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/build_data.py` around lines 6461 - 6466, The current block only
copies previous_place.address_* fields when address_country_name is missing,
which loses valid refreshed subcomponents; update the logic to preserve each
address component independently by checking each field separately (for example,
for address_country_name, address_country_code, address_admin_area,
address_locality, address_postal_code) and assign refreshed_place.<field> =
previous_place.<field> only when refreshed_place.<field> is empty/falsey and
previous_place.<field> is present, using individual if checks for
refreshed_place and previous_place on each named field.
michaelmwu
left a comment
michaelmwu
left a comment
There was a problem hiding this comment.
This is a Codex generated review. Please validate.
Thanks for taking a crack at this. I think the contribution is directionally useful, but I would not merge it as-is.
The useful part is the Google Places addressComponents extraction. That is still relevant even after the translation-memory work: translation memory helps produce English-readable display addresses from string fields, but #16 is about machine-trustworthy canonical country/address metadata from the API. Structured API components are a better source for country/admin/locality/postal-code inference than translated address strings.
The parts I would change before merging:
formatted_address_enmostly duplicates the currentformatted_address/address_display_enpath onmain, where API lookups already request English and display normalization already prefersaddress_display_enwhere available.formatted_address_localis added but not populated, so this does not yet satisfy the local-language preservation goal from #16.- The new
places.addressComponentsfield mask needs cache invalidation/versioning. Otherwise old enrichment cache entries can remain valid but never gain the newly requested fields. This probably belongs in the enrichment input signature/policy payload. - The preservation logic should copy each address component independently. A refreshed result could have a new country but be missing locality/postal code, and the current all-or-nothing block would drop useful previous subfields.
- The
pytest>=9.0.3and largeuv.lockrewrite look unrelated to this feature and should be removed or minimized.
Suggested path: keep the core idea, but narrow the patch to API address component extraction + model fields + country inference fallback + focused tests + explicit enrichment cache invalidation. I would skip formatted_address_en/local for now unless this PR also implements true dual-language API lookups.
|
Thanks @michaelmwu -- will make these changes! |
2 participants
Description
My first crack at this, heavily supported by Claude. Please have a look and let me know what you think. Thanks!
Related Issue
#16
How Has This Been Tested?
Changes made to automatic tests
Summary by CodeRabbit
New Features
Tests
Chores