fix(geocode): index-load only supports the prebuilt cities15000 index#3883
Merged
Conversation
Only the cities15000 Geonames index is prebuilt and published to the qsv GitHub releases. The index-load numeric shortcut and its USAGE text incorrectly advertised 500/1000/5000 as well, so `index-load 5000` would pass validation and then fail with an opaque 404 download error. Restrict the numeric shortcut to 15000 in both check_index_file() and load_engine_data(), returning a clear error that points users at `index-update --cities-url` for other city sets. Update the USAGE docstrings accordingly and regenerate docs/help/geocode.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 9 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Restructure the cityrecord selection in format_result() from an `is_none()` check followed by `.unwrap()` into an `if let Some(...)` match, swapping the branches accordingly. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aligns qsv geocode index-load behavior and documentation with what is actually published in qsv GitHub releases by restricting the numeric shortcut to the only prebuilt Geonames index (cities15000), and improves the suggest-path admin1 filtering control flow.
Changes:
- Restrict
index-loadnumeric shortcuts to15000with earlier, clearer validation errors. - Narrow the numeric-shortcut download branch in
load_engine_data()to15000and update CLI USAGE/doc output accordingly. - Refactor
search_index()suggest logic to avoidunwrap()when handlingadmin1_filter_list.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/cmd/geocode.rs |
Updates numeric-shortcut validation/download behavior for index-load and refactors suggest search filtering. |
docs/help/geocode.md |
Regenerates help docs to reflect the updated index-load numeric shortcut behavior. |
Addresses Copilot review on PR #3883: - load_engine_data() detected the numeric shortcut from the file *stem*, so a real local file like `15000.rkyv` (stem `15000`) was treated as the download shortcut and silently overwritten. Only treat input as the shortcut when it has no file extension. - load_engine_data() compared the raw stem string to "15000" while check_index_file() compared the parsed u16 to 15000, so `015000` was accepted by one and rejected by the other. Both now compare the parsed numeric value (against DEFAULT_GEONAMES_CITIES_INDEX), and the download URL uses the numeric value too. - Add tests/test_geocode.rs coverage: index-load with a non-15000 number fails with the actionable error, and a numeric-named .rkyv file is loaded (not overwritten by a download). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Only the cities15000 Geonames index is prebuilt and published to the qsv GitHub releases. The
index-loadnumeric shortcut and its USAGE text, however, advertised500,1000,5000and15000. As a result,qsv geocode index-load 5000would pass validation and then fail with an opaque 404 download error.This PR restricts the
index-loadnumeric shortcut to15000only:check_index_file()now rejects non-15000numeric shortcuts early with a clear, actionable message.load_engine_data()numeric branch narrowed to15000.index-* loaddescription and the<index-file>argument) updated;docs/help/geocode.mdregenerated.The error messages point users at
index-update --cities-urlfor other city sets (Geonames publishescities500/1000/5000/15000.zip, so the--cities-urlnumeric shortcut forindex-updateremains valid and is unchanged).Testing
cargo build --locked --bin qsv -F all_features— cleancargo test --test tests -F all_features geocode— 46 passed, 26 ignored (MaxMind-license tests)cargo clippy— no new warnings (one pre-existingunnecessary_unwrapwarning in unrelated code atgeocode.rs:2347left untouched)🤖 Generated with Claude Code