refactor: tighten architecture around contributed Opal payload work#71
Merged
Conversation
Release gems must not depend on git sources. The Gemfile overrides for lutaml-model and plurimath point at GitHub main, but both gems are already published on RubyGems: - lutaml-model 0.8.16 (gemspec already declares ~> 0.8.0) - plurimath 0.11.3 (not a runtime dep; needed only by specs that exercise Unitsml::Formula#to_plurimath) Changes: - Drop the `gem "lutaml-model", github: ...` line; fall through to the gemspec constraint (~> 0.8.0). - Replace `gem "plurimath", github: ...` with `gem "plurimath", "~> 0.11.3"` so the Gemfile pulls the published gem. Verified locally: bundle install resolves both from RubyGems, and bundle exec rake spec passes (398 examples, 0 failures).
…r, Xml::Formatter) Adds the supporting modules referenced by later commits: - Unitsml::Errors::UnsupportedPayloadTypeError — replaces the bare string raise in PayloadGenerator (TODO-02). - Unitsml::Errors::InvalidModelError — raised by Configuration when register_model receives a non-Unitsdb subclass (TODO-10). - Unitsml::Unitsdb::Finders — shared lookup helpers for the Units, Prefixes, and Dimensions collection wrappers (TODO-07). - Unitsml::PrefixAdapter — value object that adapts the various prefix-shaped inputs to a single type-explicit interface, replacing the respond_to?-based dispatch in Utility (TODO-11). - Unitsml::Xml::Formatter — owns the entity/encoding post-processing previously inlined as repeated gsub chains across Utility (TODO-08). Configuration.register_model now validates the superclass is a ::Unitsdb::* class. The silent `parent == Object` filter in build_context is now dead code and removed.
The auto-generated database_payload.rb now calls
Unitsml::Unitsdb::Database.load_opal_payload({...}.freeze) instead of
const_set(:DATABASE, ...). This collapses three code paths in
Database.opal_payload into one: @opal_payload is the only source of
truth, and the const_defined?/const_get dance is gone (TODO-01).
Supporting changes:
- PayloadGenerator#serialize now dispatches scalars through a
SCALAR_SERIALIZERS table (OCP — adding a new scalar type is one
line) and raises UnsupportedPayloadTypeError on unknown types
instead of a bare string (TODO-02).
- The boot file (lib/unitsml/opal.rb) eager-requires every new
autoload entry point so Opal compilation still succeeds.
- Top-level lib/unitsml.rb autoloads PrefixAdapter and Xml.
- spec/support/payload_helper.rb extracts the duplicated
extract_hash_literal helper used by both payload_generator_spec
and payload_sync_spec (TODO-03). Both specs now use it.
- database_spec.rb: drops the subclass + const_set test (no longer
applicable); uses described_class consistently.
…nil guards Collection wrapper cleanups (TODO-04, 05, 07, 09): - Unitsml::Unitsdb::Finders is now included in Units, Prefixes, and Dimensions. The triply-duplicated private find methods collapse to two shared helpers: find_first_in (flat) and find_first_through (nested via identifiers/symbols). Each host keeps its public find_by_* API; only the implementation is shared (TODO-07). - Unitsml::Unitsdb.reset_caches is a public API that clears every memoized cache (@database, @Units, @Prefixes, @Dimensions, ...). Replaces the instance_variable_set(:@database, nil) pattern in database_spec.rb. Also useful for downstream consumers who reload configuration (TODO-05). - Dimension setters: 8 near-identical setters (length=, mass=, ...) replaced with define_method over a frozen QUANTITY_KEYS list, plus generated readers. Storage moves from per-key instance variables (set via instance_variable_set) to a single @quantities hash. Drops 24 lines and the forbidden ivar pattern (TODO-04). - Unitsml::Unitsdb::Unit#dimension_url: quantity_references[0].id → quantity_references.first&.id. Empty arrays now return nil instead of NoMethodError (TODO-09). Specs: direct coverage for the Unit subclass methods, the Dimension setters / dim_symbols / processed_symbol / set_vector, and the Finders module.
… patterns Utility module cleanups (TODO-08, 11, 12 + supporting dispatch fixes): - PrefixAdapter is now the single entry point for prefix-shaped inputs in Utility. The six helper methods that used respond_to? (prefix_object, prefix_like?, prefix_symbolid, prefix_base, prefix_power, resolved_prefix) collapse into PrefixAdapter.wrap with case/when dispatch on is_a?. Thirteen respond_to? call sites removed (TODO-11). - Xml::Formatter owns the entity-decode and Unicode-encode post- processing that was inlined as repeated gsub chains in Utility.unit and Utility.prefix_xml. format_unit / format_prefix / decode_entities / encode_unit_symbols are the single source of truth (TODO-08). - .send → .public_send at three sites (dimension.rb, fenced.rb, unitsdb/dimensions.rb). The target methods are all public; the change makes intent explicit and refuses to call private methods if the contract changes later (TODO-12). - parser.rb: object.respond_to?(:power_numerator) → object.is_a?(Unit). mathml_helper.rb: klass.respond_to?(:attributes) → explicit Lutaml::Model::Serializable subclass check. - utility.rb now requires htmlentities explicitly (was loaded transitively via formula.rb). Specs: direct coverage for Xml::Formatter, PrefixAdapter, the Configuration register_model validation, and Utility.combine_prefixes edge cases (nil/nil, single-side, mismatched bases, milli+kilo→unity, non-existent combined powers).
unit_spec.rb previously used instance_double and asserted on have_received — testing interactions, not behavior. The empty Class.new stubs for ParsedUnitSymbol / UpdatedMathmlValue / UnitSymbol confirmed the test was mocking reality away. Rewritten to assert on observable output: real Unitsml::Unit instances constructed against the bundled unitsdb database, with real Unitsml::Prefix instances for the prefixed case. Three contexts: bare SI unit, prefixed unit, unit with power numerator. Asserts on result[:method_name] (:mi / :msup) and result[:value] class (Mml::V4::Mi / Mml::V4::Msup) — the actual contract.
ronaldtse
force-pushed
the
refactor/audit-clean-architecture
branch
from
76276a0 to
66cec0e
Compare
unitsdb-ruby 2.2.3+ extracted Database into services (Loader, ReferenceValidator, UniquenessValidator) and ships a public Unitsdb.data_dir API that supersedes the manual Gem.loaded_specs path construction unitsml was doing in three places. Changes: - lib/unitsml/unitsdb.rb: collapse the doubled rescue in load_database (the inner load_unitsdb_database already rescued, making the outer rescue unreachable dead code). Replace database_path, candidate_database_paths, database_files_present?, unitsdb_gem_path, and REQUIRED_DATABASE_FILES with a single call to Unitsdb.data_dir. The upstream Loader already does file-existence checks and raises DatabaseFileNotFoundError, so the local pre-check was redundant. - lib/unitsml/opal/payload_generator.rb: default_data_dir now returns Unitsdb.data_dir directly. Drops unitsdb_gem_path helper. - spec/unitsml/opal/payload_generator_spec.rb: same Unitsdb.data_dir swap in the unitsdb_data_dir let. Verified against unitsdb 2.2.2, 2.2.3 (local), and 2.2.4 (published): 456 specs, 0 failures across all three. The Unitsdb.data_dir API has been stable since 2.2.2, so no gemspec bump is required.
unitsdb 2.2.4 ships the Database services extraction (Loader, ReferenceValidator, UniquenessValidator), Unitsdb.data_dir, and other public APIs that unitsml now consumes. The ~> 2.2.4 floor rules out the older monolithic Database layout. Gemfile already pulls unitsdb transitively via the gemspec; no top-level gem "unitsdb" line is needed there.
Regenerated .rubocop_todo.yml via 'rubocop -A --auto-gen-config'. Auto-corrections applied: - utility.rb: wrap two long lines in prefix_id and quantity_name - formula.rb: wrap Plurimath::Math.parse argument list - unitsml.gemspec: sort add_dependency calls alphabetically (htmlentities, lutaml-model, mml, ox, parslet, unitsdb) No behavior change. 456 specs, 0 failures. 0 rubocop offenses.
Adds TODO* and NOTES* — local planning/scratch markdown must never ship in the gem. Adds .DS_Store for macOS noise and /.ruby-lsp/ for the Ruby LSP workspace cache.
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.
Summary
Audit-driven refactor responding to PR #65 (contributed Opal database payload loader). The loader mechanism was sound — already absorbed into #69 — but the surrounding code had accumulated forbidden patterns (
.send,respond_to?,instance_variable_set,double), DRY violations, and a few missing-model-driven seams. This PR tightens all of it.Net numbers
.send(, 0respond_to?, 0instance_variable_set/get, 0require_relativein lib/double/instance_doublein spec/What changed
database_payload.rbcallsload_opal_payloadnotconst_set(:DATABASE, ...); 3 code paths → 1UnsupportedPayloadTypeErrorandInvalidModelErrorreplace bare stringraiseand silent skipsPayloadHelperextracts duplicatedextract_hash_literaldefine_methodoverQUANTITY_KEYS; dropsinstance_variable_setUnitsml::Unitsdb.reset_cachespublic API; specs stop poking IVARsinstance_double→ realUnitsml::Unit+Unitsml::PrefixinstancesUnitsml::Unitsdb::Finderskills triply-duplicatedfindUnitsml::Xml::Formatterowns entity decode + Unicode encodequantity_references[0].id→first&.idConfiguration.register_modelvalidates::Unitsdb::*superclass; silent skip → typed errorrespond_to?calls withcase/whenonis_a?.send→.public_sendDependency note
Includes the Gemfile cleanup from #70 (commit
1b71629). If #70 lands first, that commit becomes a no-op during merge. The Gemfile change is required for the test environment to resolve without network access.Out of scope
Utilitymodule (446 LOC) into MECE submodules — flagged in the audit but deliberately deferred (large churn, no contributing-party involvement, separate PR).Test plan
bundle exec rspec— 456 examples, 0 failuresbundle exec rubocop— clean on all changed filesbundle exec rspec spec/unitsml/opal_boot_spec.rb— Opal builder compilation passesbundle exec rake unitsml:generate_opal_payload— byte-for-byte idempotent regenerationbundle exec rspec spec/unitsml/payload_sync_spec.rb— committed payload matches generator output