refactor: split converter/elements.py into a package with scoped conversion scale (v0.5.2) - #245
Merged
Merged
Conversation
…re split Characterization tests captured from the pre-split implementation: - direct output contract of all 7 public extractors + _dispatch_shape - image/media naming and img_counter behavior - pipeline JSON for standard 16:9 and non-standard 4:3 decks - import-path and signature compatibility surface SPEC: 20260731-2310_v0.5.2-elements-split Progress: Phase 1 (characterization) green on old implementation Next: Phase 2 scoped scale accessor
…textVar Replace the process-global set_emu_per_px() — which patched a fixed string list of sys.modules entries — with a ContextVar-backed conversion_scale() context manager + get_emu_per_px() getter. - pipeline wraps extraction in conversion_scale(slide_width); the scale is always restored on exit (normal / exception / nested) - converter internals fetch the scale once per invocation instead of copying EMU_PER_PX at import time (the copy is what forced the module patch list, which a package split would silently break) - EMU_PER_PX stays as a compat constant on the facade; internals no longer read it - tests: scoped set/restore, thread isolation, pipeline no-leak, and AST guards forbidding EMU_PER_PX imports / sys.modules patching SPEC: 20260731-2310_v0.5.2-elements-split Progress: Phase 2 (scale isolation) complete, 593 passed Next: Phase 3 elements package split
…a package
Mechanical move of the 1519-line monolith into 4 modules + facade
(bodies unchanged; only imports rewired to the parent package):
- shapes.py name maps + line / freeform / auto-shape extraction
- textbox.py textbox extraction (delegates preset geometry to shapes)
- media.py video / picture / SVG / image-fill / referenced images
- dispatch.py raw passthrough, type routing, group recursion
- __init__.py compatibility facade (7 public extractors + _dispatch_shape)
Dependency DAG (textbox->shapes, dispatch->{shapes,textbox,media}) is
enforced by an AST guard; submodule import smoke tests included.
Characterization expectations unchanged — 615 passed.
SPEC: 20260731-2310_v0.5.2-elements-split
Progress: Phase 3 (package split) complete
Next: Phase 4 warning closure
- Fix lxml FutureWarning: truth-test of grpSpPr element -> 'is not None' (identical semantics — when grpSpPr has no children, find() returns None either way) - pytest: promote FutureWarning to error so it cannot regress silently - pytest: ignore only defusedxml's own cElementTree DeprecationWarning (upstream shim on Python 3.12; message/category/module-scoped filter, the defuse_stdlib security patch itself is untouched) SPEC: 20260731-2310_v0.5.2-elements-split Progress: Phase 4 (warning closure) complete Next: final validation + PR
…lved SPEC: 20260731-2310_v0.5.2-elements-split
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
Resolves the
converter/elements.pyknown debt (principles.md): the 1519-line / 18-definition monolith is now a 4-module package with an enforced dependency DAG, and the converter's px scale is no longer process-global mutable state.Internal refactoring only — no changes to public import paths, signatures, or JSON output (guarded by characterization tests; no CHANGELOG entry per versioning policy).
1. Characterization first (safety net)
_dispatch_shape, image/media file naming, the image counter, and pipeline JSON for standard 16:9 and non-standard 4:3 decks2. Scoped conversion scale (prerequisite for the split)
set_emu_per_px()patched a fixed string list ofsys.modulesentries to overwrite import-time copies ofEMU_PER_PX. Splitting elements.py would have silently broken non-standard slide widths (new submodules missing from the patch list), and the process-global mutation already leaked across conversions (demonstrated: after converting a 4:3 deck, later direct extractor calls ran on a 4762.5 basis).Replaced with
ContextVar+conversion_scale()context manager +get_emu_per_px():EMU_PER_PXimports orsys.modulesaccess in converter internalsEMU_PER_PX = 6350stays on the facade for import compatibility3. Package split (mechanical move, bodies unchanged)
Dependency DAG (
textbox→shapes,dispatch→{shapes,textbox,media}, facade re-export only) is enforced by an AST guard; submodules never import the facade.4. Warning closure
if grp_sp_pr→is not None(identical semantics)FutureWarningto error so it cannot regressdefuse_stdlib()security patch is untouchedTesting
make all: 616 passed, 4 skipped, zero warnings (no existing test assertions modified)make smoke: OK (24 tools, 4 templates, persona served over real stdio)pytest tests/test_converter_elements.py tests/test_converter_robustness.py -W error::FutureWarning: 104 passedNot in scope
Extractor redesign (Strategy registry / ExtractionContext),
extract_textbox_elementinternal decomposition, and any schema/output changes — per the v0.5.x roadmap, one theme per release.