registry/search: warn instead of silently coercing bad limit values#196
Open
SoundMatt wants to merge 1 commit into
Open
registry/search: warn instead of silently coercing bad limit values#196SoundMatt wants to merge 1 commit into
SoundMatt wants to merge 1 commit into
Conversation
SKOSSearchService.parse_limit silently coerced unparseable strings and negative integers to 0. search_keyword treats limit_value == 0 as 'return no results', so a typo in --limit produced an empty result list indistinguishable from 'no matches found'. The user got no signal that their input was rejected. Add log.warning in the three coerce branches: - unparseable string (ValueError on int() conversion) - negative integer parsed from a string - negative integer passed directly Return values are unchanged, so the existing test_parse_limit_method assertions still pass. Three new tests cover the warning paths and verify that limit=0 (a legitimate user request for no results) does NOT warn. Switch the file from stdlib logging to the s2dm log module to match the rest of the codebase (registry/spec_history.py and elsewhere) and so the warnings are caplog-testable via logger='s2dm'. Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.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
1 participant
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.
Change
Add
log.warningin the three coerce branches:ValueErroronint()conversion)Return values are unchanged, so the existing assertions in
test_parse_limit_method(e.g.parse_limit("invalid") == 0,parse_limit(-1) == 0) still pass — this is purely additive observability, not a behaviour change.Tests
Three new tests in
test_skos_search.py:test_parse_limit_warns_on_unparseable_string— warning fires, return value still 0test_parse_limit_warns_on_negative_int— warning fires, return value still 0test_parse_limit_does_not_warn_on_zero—0and"0"are legitimate "no results requested" inputs and must NOT warnOther changes
Switched
src/s2dm/registry/search.pyfrom stdlibloggingtofrom s2dm import logto match the rest of the codebase (registry/spec_history.pyand elsewhere) and so the warnings are caplog-testable vialogger="s2dm". The existinglogging.error(...)call elsewhere in the file is left untouched (out of scope).Local verification: 535 tests pass.