fix(cli): wrap Dir.glob strings before calling sub_ext - #85
Merged
Conversation
ucode 0.5.0's code_chart extract --verify crashed with NoMethodError: undefined method 'sub_ext' for String because Dir.glob returns Strings, but the post-extract verify pass called .sub_ext(.json) directly on them. Wrap in Pathname first. Also fixes a silent bug in the same path: sidecar JSON's source_cell is string-keyed after JSON.parse, but Verifier::Result#source_cell reads with symbol keys (:x, :y). Round-tripping JSON -> Result -> Verifier without normalizing keys silently dropped source_cell to nil, making every positionally-correlated glyph return Skipped(:no_location). transform_keys(&:to_sym) in build_extractor_result_for_verification fixes the round-trip. Adds a CLI integration spec that exercises the full extract --verify path against a fixture SVG+JSON pair. Force Verifier::Builder.pick to nil so the spec runs without resvg or mutool. The spec would have caught the 0.5.0 regression; it now does.
The original 'does not crash with NoMethodError on --verify' spec called described_class.start with the full extract --verify ARGV. CI fails at extract --block Sidetic because Blocks.txt isn't present in the UCD cache on a clean runner — the spec guards the verify path but trips earlier on extract. Rewrite the spec to: 1. Call build_extractor_result_for_verification directly and assert the round-trip Result has symbol-keyed source_cell. 2. Call verify_block directly with a stubbed Verifier and assert the path reaches the verifier without raising. Both targeted specs cover the actual crash (String#sub_ext) without CI's UCD cache dependency.
The 0.5.0 release bumped lib/ucode/version.rb but missed syncing config/unicode17_universal_glyph_set.yml#ucode_version. The smoke spec that asserts the two stay in sync then fails on the next CI run because the merge of main's version.rb (0.5.0) with this branch's YAML (0.4.0) creates a mismatch in the CI checkout. Sync the YAML to match main's post-release version so the smoke spec passes on the post-0.5.0 merge.
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.
Fixes the NoMethodError: undefined method sub_ext for String crash that ucode 0.5.0 code_chart extract --verify hit on the first SVG (Dir.glob returns Strings; .sub_ext is a Pathname method).
Also fixes a silent bug in the same path: sidecar JSONs source_cell is string-keyed after JSON.parse, but Verifier::Result#source_cell is read with symbol keys. Without normalization, every positionally-correlated glyph round-tripped into Skipped(:no_location).
Test plan:
Out of scope: positional-correlation failures #57 (U+1166C) and #58 (U+116DA) are pre-existing ucode bugs in the positional matcher (not touched by this PR). Separate investigation.