fix: allow DISC_NUMBER to be optional, matching DISC_TOTAL - #82
Open
Andreas-Garcia wants to merge 1 commit into
Open
fix: allow DISC_NUMBER to be optional, matching DISC_TOTAL#82Andreas-Garcia wants to merge 1 commit into
Andreas-Garcia wants to merge 1 commit into
Conversation
DISC_NUMBER's type was int-only, so files without a disc number failed schema validation while DISC_TOTAL (also optional) validated fine. Fixing the type surfaced two more bugs: the field-schema special-case for DISC_TOTAL relied on get_optional_type() is int, which silently mis-reported DISC_NUMBER once its optional type became int | None; and get_optional_type()'s return annotation didn't match the non-type runtime values (e.g. int | float) it already returned, so the isinstance() call site needed a cast().
github-actions
Bot
added
documentation
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes unified-metadata schema/type handling so DISC_NUMBER is truly optional (nullable) in the same way as DISC_TOTAL, preventing schema validation regressions when a disc number is absent.
Changes:
- Updated
UnifiedMetadataKey.DISC_NUMBERoptional type toint | Noneand correctedget_optional_type()’s return annotation to reflect non-typeruntime values (unions / generic aliases). - Fixed field-schema generation to explicitly treat both
DISC_NUMBERandDISC_TOTALasvalue_type: "integer"withoptional_value: True. - Added/updated unit tests covering the
DISC_NUMBERschema descriptor and documented the fix inCHANGELOG.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| CHANGELOG.md | Documents the DISC_NUMBER optionality fix and the related schema/type-annotation corrections. |
| audiometa/utils/unified_metadata_key.py | Makes DISC_NUMBER nullable and corrects get_optional_type() typing to match actual returned runtime objects. |
| audiometa/utils/unified_metadata_field_schema.py | Ensures DISC_NUMBER is described as an optional integer in the wire schema, matching DISC_TOTAL. |
| audiometa/test/tests/unit/utils/test_unified_metadata_field_schema.py | Adds regression coverage for DISC_NUMBER being an optional integer in the field schema. |
| audiometa/init.py | Narrows expected_type to a plain type at the isinstance() call site via cast() to keep runtime checks valid. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
35 tasks
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.
Description
DISC_NUMBER's optional type wasint-only, so files without a disc number failed unified-metadata schema validation, whileDISC_TOTAL(also optional) validated fine.Fixing that type surfaced two more bugs:
unified_metadata_field_schema.pyspecial-casedDISC_TOTALto reportvalue_type: "integer"/optional_value: True, but relied onget_optional_type() is intforDISC_NUMBER— onceDISC_NUMBER's optional type becameint | None, that check silently fell through to"string"/optional_value: False.DISC_NUMBERnow sharesDISC_TOTAL's explicit branch.UnifiedMetadataKey.get_optional_type()was annotated as returningtype[int | float | str | list[str]], but its map already returned non-typeruntime objects (e.g.int | float, and nowint | None). Corrected the return annotation toobjectand narrowed withcast()at the one call site that requires a plaintypeforisinstance().Split out of #80 as a standalone, independently-mergeable library bug fix (unrelated to that PR's content changes).
Type of Change
Tests
pytest audiometa/test/tests/unit/utils/test_unified_metadata_field_schema.py— 10 passed