Introduce context-first configuration and register Unitsdb model classes#25
Introduce context-first configuration and register Unitsdb model classes#25
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Introduces a context-first configuration system for Unitsdb and registers model classes through Unitsdb::Configuration, while updating database loading to be context-aware and more strictly validated.
Changes:
- Added
Unitsdb::Configurationfor model registration, context creation, and substitution resolution. - Refactored
Unitsdb::Database.from_dbto validate/compose YAML inputs and to load within aGlobalContextcontext. - Updated Unitsdb model files to self-register via
Configuration.register_model, and added specs for context usage and improved load behavior.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/unitsdb/database_spec.rb | Adds specs for stdout silence and clearer load-time failure cases (invalid YAML structure, missing collection key). |
| spec/unitsdb/configuration_spec.rb | Adds a focused spec validating custom context + substitution + explicit registration behavior. |
| lib/unitsdb/units.rb | Registers Units model in the configuration registry. |
| lib/unitsdb/unit_systems.rb | Registers UnitSystems model in the configuration registry. |
| lib/unitsdb/unit_system_reference.rb | Registers UnitSystemReference model in the configuration registry. |
| lib/unitsdb/unit_system.rb | Registers UnitSystem model in the configuration registry. |
| lib/unitsdb/unit_reference.rb | Registers UnitReference model in the configuration registry. |
| lib/unitsdb/unit.rb | Registers Unit model in the configuration registry. |
| lib/unitsdb/ucum.rb | Registers UCUM-related models in the configuration registry. |
| lib/unitsdb/symbol_presentations.rb | Registers SymbolPresentations model in the configuration registry. |
| lib/unitsdb/si_derived_base.rb | Registers SiDerivedBase model in the configuration registry. |
| lib/unitsdb/scales.rb | Registers Scales model in the configuration registry. |
| lib/unitsdb/scale_reference.rb | Registers ScaleReference model in the configuration registry. |
| lib/unitsdb/scale_properties.rb | Registers ScaleProperties model in the configuration registry. |
| lib/unitsdb/scale.rb | Registers Scale model in the configuration registry. |
| lib/unitsdb/root_unit_reference.rb | Registers RootUnitReference model in the configuration registry. |
| lib/unitsdb/qudt.rb | Registers QUDT-related models in the configuration registry. |
| lib/unitsdb/quantity_reference.rb | Registers QuantityReference model in the configuration registry. |
| lib/unitsdb/quantity.rb | Registers Quantity model in the configuration registry. |
| lib/unitsdb/quantities.rb | Registers Quantities model in the configuration registry. |
| lib/unitsdb/prefixes.rb | Registers Prefixes model in the configuration registry. |
| lib/unitsdb/prefix_reference.rb | Registers PrefixReference model in the configuration registry. |
| lib/unitsdb/prefix.rb | Registers Prefix model in the configuration registry. |
| lib/unitsdb/localized_string.rb | Registers LocalizedString model in the configuration registry. |
| lib/unitsdb/identifier.rb | Registers Identifier model in the configuration registry. |
| lib/unitsdb/external_reference.rb | Registers ExternalReference model in the configuration registry. |
| lib/unitsdb/dimensions.rb | Registers Dimensions model in the configuration registry. |
| lib/unitsdb/dimension_reference.rb | Registers DimensionReference model in the configuration registry. |
| lib/unitsdb/dimension_details.rb | Registers DimensionDetails model in the configuration registry. |
| lib/unitsdb/dimension.rb | Registers Dimension model in the configuration registry. |
| lib/unitsdb/database.rb | Refactors DB loading: centralizes file list, validates schema versions/collections, improves error handling, adds context-based load wrapper, registers Database. |
| lib/unitsdb/configuration.rb | Adds the new configuration/context system including registry building and substitution resolution. |
| lib/unitsdb/config.rb | Removes legacy Unitsdb::Config model lookup mechanism. |
| lib/unitsdb.rb | Requires configuration, adjusts autoloads for Opal, makes Unitsdb.database context-aware with per-context caching. |
| Gemfile | Points lutaml-model to a specific GitHub branch for global-context behavior changes. |
Comments suppressed due to low confidence (1)
spec/unitsdb/database_spec.rb:1
ENV["DEBUG"] = original_debugwill raise aTypeErrorwhenoriginal_debugisnil(common when DEBUG was not set). Restore the env var by deleting it whenoriginal_debugisnil, otherwise reassign it.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Lutaml::Model::GlobalContext.with_context(context_id) do | ||
| from_hash(combined_hash, register: context_id) | ||
| end |
There was a problem hiding this comment.
The PR description says register: should be passed “only when an explicit register exists”, but this call passes register: context_id unconditionally. If the intent is to avoid implicitly creating/using a register, update this to only provide register: when a register has been explicitly populated/declared for that context (and omit the keyword entirely otherwise).
| def self.validate_schema_versions!(documents) | ||
| versions = DATABASE_FILES.each_with_object({}) do |(collection_key, filename), result| | ||
| document = documents.fetch(collection_key) | ||
| result[filename] = document.fetch("schema_version") | ||
| rescue KeyError | ||
| raise Errors::DatabaseFileInvalidError, | ||
| "Missing schema_version in files: #{missing_schema.join(', ')}" | ||
| "Missing schema_version in #{filename}" | ||
| end | ||
|
|
||
| # Extract versions from each file | ||
| prefixes_version = prefixes_hash["schema_version"] | ||
| dimensions_version = dimensions_hash["schema_version"] | ||
| units_version = units_hash["schema_version"] | ||
| quantities_version = quantities_hash["schema_version"] | ||
| unit_systems_version = unit_systems_hash["schema_version"] | ||
|
|
||
| # Check if all versions match | ||
| versions = [ | ||
| prefixes_version, | ||
| dimensions_version, | ||
| units_version, | ||
| quantities_version, | ||
| unit_systems_version, | ||
| ] | ||
|
|
||
| unless versions.uniq.size == 1 | ||
| version_info = { | ||
| "prefixes.yaml" => prefixes_version, | ||
| "dimensions.yaml" => dimensions_version, | ||
| "units.yaml" => units_version, | ||
| "quantities.yaml" => quantities_version, | ||
| "unit_systems.yaml" => unit_systems_version, | ||
| } | ||
| unless versions.values.uniq.size == 1 | ||
| raise Errors::VersionMismatchError, | ||
| "Version mismatch in database files: #{version_info.inspect}" | ||
| "Version mismatch in database files: #{versions.inspect}" | ||
| end | ||
|
|
||
| # Check if the version is supported | ||
| version = versions.first | ||
| unless version == "2.0.0" | ||
| version = versions.values.first | ||
| unless version == SUPPORTED_SCHEMA_VERSION | ||
| raise Errors::UnsupportedVersionError, | ||
| "Unsupported database version: #{version}. Only version 2.0.0 is supported." | ||
| "Unsupported database version: #{version}. Only version #{SUPPORTED_SCHEMA_VERSION} is supported." | ||
| end | ||
|
|
||
| combined_yaml = { | ||
| "schema_version" => prefixes_version, | ||
| "prefixes" => prefixes_hash["prefixes"], | ||
| "dimensions" => dimensions_hash["dimensions"], | ||
| "units" => units_hash["units"], | ||
| "quantities" => quantities_hash["quantities"], | ||
| "unit_systems" => unit_systems_hash["unit_systems"], | ||
| }.to_yaml | ||
| version | ||
| end |
There was a problem hiding this comment.
This introduces new, user-facing validation paths (missing schema_version, version mismatch across files, and unsupported schema version) without corresponding specs. Add tests in spec/unitsdb/database_spec.rb that exercise each raised error and verify the error class/message match expectations (similar to the newly added “not a mapping” and “missing collection key” specs).
There was a problem hiding this comment.
@suleman-uzair this seems to require a revert? Or is this because of Opal? The point is to check all of the schema files to ensure they have the same version.
There was a problem hiding this comment.
@ronaldtse I do not think this needs a revert. I refactored the structure here, but I did not intend to change the version validation or the other existing behavior in this method. If you had a specific case in mind that looks different after the refactor, let me know and I'll look into it.
| end | ||
|
|
||
| def context(id = context_id, force_populate: false) | ||
| return find_context(id) if find_context(id) && !force_populate && populated_for(id) |
There was a problem hiding this comment.
find_context(id) is called twice, which is unnecessary and makes the early-return condition harder to read. Store it in a local variable (e.g., existing = find_context(id)) and use that for the conditional/return to reduce repeated lookups and improve clarity.
| return find_context(id) if find_context(id) && !force_populate && populated_for(id) | |
| existing = find_context(id) | |
| return existing if existing && !force_populate && populated_for(id) |
Summary
This change introduces a context-first
Unitsdb::Configurationflow and wires model registration throughConfiguration.register_model.What changed
Unitsdb::Configurationwith:register_modelcontext/populate_contextregister/populate_registerUnitsdb.databaseto be context-aware and cache by context ID.Unitsdb::Database.from_dbto:context:GlobalContext.with_context(context_id)register:only when an explicit register existsConfiguration.register_model(...)entries across Unitsdb model files, including:Current behavior/design intent