Raise an informative error when a fetch query matches no files - #1085
Raise an informative error when a fetch query matches no files#1085lobennett wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for opening this pull request! We have detected this is the first time you have contributed to NiMARE. Please check out our contributing guidelines. Of course, if you want to opt out this time there is no problem at all with adding your name later. You will be always welcome to add it in the future whenever you feel it should be listed. |
Reviewer's GuideAdds pre-download validation of fetch queries in the extraction utilities so unmatched queries raise an informative ValueError listing available entity options, via a new helper that parses the database manifest, along with unit tests covering the helper and the new error behavior for neurosynth and neuroquery fetchers. Sequence diagram for fetch_neurosynth validation and error on unmatched querysequenceDiagram
actor User
participant FetchNeurosynth
participant _fetch_database
participant _get_available_entities
User->>FetchNeurosynth: fetch_neurosynth(vocab, return_type)
FetchNeurosynth->>_fetch_database: _fetch_database(search_pairs, database_url, out_dir, overwrite)
_fetch_database->>_fetch_database: scan database_file_manifest for search_pairs
alt no_matching_databases
_fetch_database->>_get_available_entities: _get_available_entities(database_file_manifest, data)
_get_available_entities-->>_fetch_database: available_entities
_fetch_database-->>User: ValueError
else matching_databases_found
_fetch_database-->>FetchNeurosynth: found_files
FetchNeurosynth-->>User: data_or_files
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
_fetch_database, the error message formatsdata-{search_pairs.get('data')}even whendatamight beNoneor when no available entities are found, which can lead to awkward messages likedata-Noneor an empty options list; consider guarding this formatting and falling back to a more generic message in those cases. - The
_get_available_entitiesparser assumes entity segments are separated by underscores and always inkey-valueform (e.g.,version-7), which tightly couples it to the current naming convention; consider constraining parsing to known keys or using a regex so that unexpected filename changes or additional underscores do not silently produce incorrect entity mappings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_fetch_database`, the error message formats `data-{search_pairs.get('data')}` even when `data` might be `None` or when no available entities are found, which can lead to awkward messages like `data-None` or an empty options list; consider guarding this formatting and falling back to a more generic message in those cases.
- The `_get_available_entities` parser assumes entity segments are separated by underscores and always in `key-value` form (e.g., `version-7`), which tightly couples it to the current naming convention; consider constraining parsing to known keys or using a regex so that unexpected filename changes or additional underscores do not silently produce incorrect entity mappings.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Address review feedback on PR neurostuff#1085: - Constrain _get_available_entities to a known-key allowlist (data, version, vocab, source, type) so an unexpected key-value segment in a feature filename can no longer silently mismap entities. - Guard the _fetch_database no-match message: fall back to a generic scope ("the requested database") when the query has no data key (avoids "data-None"), and emit a generic message pointing at the manifest when no entity options are available. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed both review points in 538fb9a:
Added three tests (unknown-segment ignored, |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1085 +/- ##
==========================================
+ Coverage 85.50% 85.54% +0.03%
==========================================
Files 56 56
Lines 11248 11274 +26
==========================================
+ Hits 9618 9644 +26
Misses 1630 1630 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fetch_neurosynth/fetch_neuroquerypassed an unmatched query straight through_fetch_database, which returned an empty list and silently yielded no data — so a typo'dsource/vocab/typelooked like an empty result with no hint.This raises an informative
ValueErrorwhen a query matches no files in the bundled manifest, listing the available values per entity (version/vocab/source/type) for the requested data source. The match happens before any download, so the error is immediate. Adds a small pure helper (_get_available_entities) with unit tests, plus offline tests that the fetch functions raise.Closes #640
Before / after
Before:
After:
Summary by Sourcery
Raise an informative error when metadata fetch queries match no files in bundled manifests and surface available query options.
Bug Fixes:
Enhancements:
Tests: