Skip to content

refactor: encapsulation cleanup (send/respond_to?) and complete performance TODOs#694

Open
ronaldtse wants to merge 1 commit into
mainfrom
refactor/encapsulation-and-perf-todos
Open

refactor: encapsulation cleanup (send/respond_to?) and complete performance TODOs#694
ronaldtse wants to merge 1 commit into
mainfrom
refactor/encapsulation-and-perf-todos

Conversation

@ronaldtse
Copy link
Copy Markdown
Contributor

Summary

This PR completes two major workstreams:

1. Encapsulation Cleanup

  • send()public_send(): All dynamic dispatch now respects method visibility
  • respond_to?is_a?: Type checks use explicit type comparison instead of duck typing
  • Test doubles: Updated anonymous classes to inherit from Lutaml::Xml::Namespace for proper type checking
  • Documented edge cases: respond_to? kept where method is dynamically added (e.g., element_order on non-Serialize models)

2. Performance TODOs (all verified and removed)

TODO.remaining-perf (7 items — all verified DONE):

  • 01: Truly lazy namespace collection — stores element reference instead of eager recursive walk
  • 02: Short-circuit UninitializedClass in XML path (key_value path uses new with different nil semantics)
  • 03: Replace respond_to? with type hierarchy — removed redundant is_a?(XmlElement) checks on children
  • 04: Thread resolved type through call chain, cache resolver in Attribute
  • 05: Concurrent::Map for @mappings (not compute_if_absent due to recursive deadlock risk)
  • 06: Cache prefixed element names, reduce options.except/merge allocations
  • 07: Thread instance_is_serialize flag instead of checking is_a?(Serialize) 8+ times per element

TODO.pi-phase1 (6 items — 4 verified DONE, 2 correctly deferred):

  • 1: Multi-adapter roundtrip tests ✓
  • 2: Hash attribute default nil handling ✓
  • 3: Value map/transform for PIs — deferred (no use case)
  • 4: Inline processing instructions — deferred (requires mixed_content)
  • 5: RuboCop compliance ✓
  • 6: Documentation ✓

Test Results

  • 4931 examples, 0 failures
  • All 3 remaining RuboCop offenses are pre-existing (block length in transformation.rb, safe nav in adapter_resolver.rb, verifying doubles in optimization_spec.rb)

Files Changed

  • 81 files changed, 706 insertions, 381 deletions

…e performance TODOs

Encapsulation cleanup:
- Replace all send() calls with public_send() to respect method visibility
- Replace respond_to? with explicit is_a? type checks for proper typing
- Remove instance_variable_get on external objects where found
- Update test doubles to use proper Namespace subclasses

Performance optimizations (verified from TODO.remaining-perf):
- 01: Truly lazy namespace collection (store element ref instead of eager walk)
- 02: Short-circuit UninitializedClass in XML path (key_value path unchanged)
- 03: Replace respond_to? with type hierarchy in hot path
- 04: Thread resolved type through call chain, cache resolver in Attribute
- 05: Replace Mutex with Concurrent::Map in TransformationRegistry
- 06: Reduce GC pressure (cache prefixed names, avoid unnecessary allocations)
- 07: Eliminate repeated is_a?(Serialize) checks with instance flag

PI Phase 1 (verified from TODO.pi-phase1):
- Multi-adapter roundtrip tests verified
- Hash attribute default nil handling verified
- Documentation verified
- Items 3 (value_map for PIs) and 4 (inline PIs) correctly deferred

All 4931 tests pass, 0 failures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant