From 694c65d93a2b04e7358272789aa6e03914e879c7 Mon Sep 17 00:00:00 2001 From: Michael Holloway Date: Thu, 6 Feb 2025 21:59:24 -0500 Subject: [PATCH] Fix up polymorphism for core model types (#5) * Fix up polymorphism for core model types Previously the manuscript, holding, and record classes weren't doing much. They were added with an eye toward polymorphism, but the ExportRepresenter was converting all item hashes unconditionally to a generic DsItem object with methods applicable to all three. This change gets ExportRepresenter working as intended so that Holding, Manuscript, and Record objects are created by ExportRepresenter where appropriate. DsItem is removed as no longer needed, and all unused methods of the core model types are removed as well. The spec helper export hash is also updated to accommodate these changes. Addresses comment on #3 * Apply suggestions from code review Co-authored-by: emeryr-upenn <135385485+emeryr-upenn@users.noreply.github.com> * Revive DsItem, add instance_of method * cleanup --------- Co-authored-by: emeryr-upenn <135385485+emeryr-upenn@users.noreply.github.com> --- digital_scriptorium.gemspec | 1 + lib/digital_scriptorium/ds_item.rb | 42 ++----------- lib/digital_scriptorium/ds_meta.rb | 6 +- lib/digital_scriptorium/export.rb | 7 +++ lib/digital_scriptorium/export_representer.rb | 14 ++++- lib/digital_scriptorium/holding.rb | 25 +++----- lib/digital_scriptorium/item_id.rb | 4 +- lib/digital_scriptorium/record.rb | 60 ------------------- spec/digital_scriptorium/ds_item_spec.rb | 28 ++++----- spec/digital_scriptorium/ds_meta_spec.rb | 7 +-- .../export_representer_spec.rb | 27 ++++----- spec/spec_helper.rb | 46 +++++--------- 12 files changed, 73 insertions(+), 194 deletions(-) diff --git a/digital_scriptorium.gemspec b/digital_scriptorium.gemspec index 220d655..a276665 100644 --- a/digital_scriptorium.gemspec +++ b/digital_scriptorium.gemspec @@ -30,6 +30,7 @@ Gem::Specification.new do |spec| spec.add_dependency 'wikibase_representable', '~> 0.1' spec.add_development_dependency 'bundler', '~> 2.5' + spec.add_development_dependency 'pry', '~> 0.14' spec.add_development_dependency 'rake', '~> 13.2' spec.add_development_dependency 'rspec', '~> 3.13' spec.add_development_dependency 'upennlib-rubocop', '~> 1.2' diff --git a/lib/digital_scriptorium/ds_item.rb b/lib/digital_scriptorium/ds_item.rb index 7006434..3694ce5 100644 --- a/lib/digital_scriptorium/ds_item.rb +++ b/lib/digital_scriptorium/ds_item.rb @@ -3,46 +3,12 @@ require 'wikibase_representable' module DigitalScriptorium - # Represents a Digital Scriptorium item + # Represents a generic Digital Scriptorium item class DsItem < WikibaseRepresentable::Model::Item - def instance_of_claims - claims_by_property_id PropertyId::INSTANCE_OF # P16 - end - - def ds_id - claims_by_property_id(PropertyId::DS_ID)&.first&.data_value # P1 - end - - def holding_ids - claims_by_property_id(PropertyId::MANUSCRIPT_HOLDING)&.map(&:entity_id_value) # P2 - end - - def described_manuscript_id - claims_by_property_id(PropertyId::DESCRIBED_MANUSCRIPT)&.first&.entity_id_value # P3 - end - - def holding_status - claims_by_property_id(PropertyId::HOLDING_STATUS)&.first&.entity_id_value # P6 - end - - def iiif_manifest - claims_by_property_id(PropertyId::IIIF_MANIFEST)&.first&.entity_id_value # P41 - end - - def core_model_item? - instance_of_claims.any? { |claim| ItemId::CORE_MODEL_ITEMS.include? claim.entity_id_value } - end - - def manuscript? - instance_of_claims.any? { |claim| claim.entity_id_value == ItemId::MANUSCRIPT } - end - - def holding? - instance_of_claims.any? { |claim| claim.entity_id_value == ItemId::HOLDING } - end + include PropertyId - def record? - instance_of_claims.any? { |claim| claim.entity_id_value == ItemId::RECORD } + def instance_of + claims_by_property_id(INSTANCE_OF)&.first&.entity_id_value end end end diff --git a/lib/digital_scriptorium/ds_meta.rb b/lib/digital_scriptorium/ds_meta.rb index 2ab2cf2..c13acd6 100644 --- a/lib/digital_scriptorium/ds_meta.rb +++ b/lib/digital_scriptorium/ds_meta.rb @@ -21,12 +21,8 @@ def initialize(record, export_hash) @record = record end - def current?(holding) - holding.holding_status == HOLDING_STATUS_CURRENT - end - def current_holdings(manuscript, export_hash) - manuscript.holding_ids.filter_map { |id| export_hash[id] if current?(export_hash[id]) } + manuscript.holding_ids.filter_map { |id| export_hash[id] if export_hash[id]&.current? } end end end diff --git a/lib/digital_scriptorium/export.rb b/lib/digital_scriptorium/export.rb index 01e237a..70b43da 100644 --- a/lib/digital_scriptorium/export.rb +++ b/lib/digital_scriptorium/export.rb @@ -11,5 +11,12 @@ def to_hash end hash end + + def instance_of_id_from(item_hash) + claims = item_hash['claims'] + return nil unless claims&.any? + + claims.dig('P16', 0, 'mainsnak', 'datavalue', 'value', 'id') + end end end diff --git a/lib/digital_scriptorium/export_representer.rb b/lib/digital_scriptorium/export_representer.rb index 231b44b..25fedbb 100644 --- a/lib/digital_scriptorium/export_representer.rb +++ b/lib/digital_scriptorium/export_representer.rb @@ -6,6 +6,7 @@ module DigitalScriptorium # Representer class for deserializing Wikibase data exports from JSON. class ExportRepresenter < Representable::Decorator + include ItemId include Representable::JSON::Collection include WikibaseRepresentable::Model include WikibaseRepresentable::Representers @@ -13,7 +14,18 @@ class ExportRepresenter < Representable::Decorator items decorator: lambda { |input:, **| input.type == Item::ENTITY_TYPE ? ItemRepresenter : PropertyRepresenter }, class: lambda { |input:, **| - input['type'] == Item::ENTITY_TYPE ? DsItem : Property + return Property unless input['type'] == Item::ENTITY_TYPE + + case instance_of_id_from input + when MANUSCRIPT + Manuscript + when HOLDING + Holding + when DS_20_RECORD + Record + else + DsItem + end } end end diff --git a/lib/digital_scriptorium/holding.rb b/lib/digital_scriptorium/holding.rb index 4a407a0..ce95845 100644 --- a/lib/digital_scriptorium/holding.rb +++ b/lib/digital_scriptorium/holding.rb @@ -3,32 +3,21 @@ module DigitalScriptorium # An item representing a Digital Scriptorium holding (instance of Q2) class Holding < DsItem - def institution_as_recorded_claims - claims_by_property_id HOLDING_INSTITUTION_AS_RECORDED # P5 - end + include ItemId + include PropertyId def status_claims claims_by_property_id HOLDING_STATUS # P6 end - def institutional_id_claims - claims_by_property_id INSTITUTIONAL_ID # P7 - end - - def shelfmark_claims - claims_by_property_id SHELFMARK # P8 - end - - def link_to_institutional_record_claims - claims_by_property_id LINK_TO_INSTITUTIONAL_RECORD # P9 - end + def status + return unless status_claims&.any? - def start_time_claims - claims_by_property_id START_TIME # P38 + status_claims&.first&.entity_id_value end - def end_time_claims - claims_by_property_id END_TIME # P39 + def current? + status == HOLDING_STATUS_CURRENT end end end diff --git a/lib/digital_scriptorium/item_id.rb b/lib/digital_scriptorium/item_id.rb index 35328a5..8bc3f36 100644 --- a/lib/digital_scriptorium/item_id.rb +++ b/lib/digital_scriptorium/item_id.rb @@ -7,7 +7,7 @@ module DigitalScriptorium module ItemId MANUSCRIPT = 'Q1' HOLDING = 'Q2' - RECORD = 'Q3' + DS_20_RECORD = 'Q3' HOLDING_STATUS_CURRENT = 'Q4' HOLDING_STATUS_NON_CURRENT = 'Q5' STANDARD_TITLE = 'Q6' @@ -23,6 +23,6 @@ module ItemId PLACE = 'Q16' MATERIAL = 'Q17' - CORE_MODEL_ITEMS = Set[MANUSCRIPT, HOLDING, RECORD] + CORE_MODEL_ITEMS = Set[MANUSCRIPT, HOLDING, DS_20_RECORD] end end diff --git a/lib/digital_scriptorium/record.rb b/lib/digital_scriptorium/record.rb index a80651b..1772860 100644 --- a/lib/digital_scriptorium/record.rb +++ b/lib/digital_scriptorium/record.rb @@ -8,65 +8,5 @@ class Record < DsItem def described_manuscript_id claims_by_property_id(DESCRIBED_MANUSCRIPT)&.first&.entity_id_value # P3 end - - def title_as_recorded_claims - claims_by_property_id TITLE_AS_RECORDED # P10 - end - - def uniform_title_as_recorded_claims - claims_by_property_id UNIFORM_TITLE_AS_RECORDED # P12 - end - - def associated_name_as_recorded_claims - claims_by_property_id ASSOCIATED_NAME_AS_RECORDED # P14 - end - - def genre_as_recorded_claims - claims_by_property_id GENRE_AS_RECORDED # P18 - end - - def language_as_recorded_claims - claims_by_property_id LANGUAGE_AS_RECORDED # P21 - end - - def production_date_as_recorded_claims - claims_by_property_id PRODUCTION_DATE_AS_RECORDED # P23 - end - - def dated_claims - claims_by_property_id DATED # P26 - end - - def production_place_as_recorded_claims - claims_by_property_id PRODUCTION_PLACE_AS_RECORDED # P27 - end - - def physical_description_claims - claims_by_property_id PHYSICAL_DESCRIPTION # P29 - end - - def material_as_recorded_claims - claims_by_property_id MATERIAL_AS_RECORDED # P30 - end - - def note_claims - claims_by_property_id NOTE # P32 - end - - def acknowledgements_claims - claims_by_property_id ACKNOWLEDGEMENTS # P33 - end - - def date_added_claims - claims_by_property_id DATE_ADDED # P34 - end - - def date_last_updated_claims - claims_by_property_id DATE_LAST_UPDATED # P35 - end - - def iiif_manifest_claims - claims_by_property_id IIIF_MANIFEST # P41 - end end end diff --git a/spec/digital_scriptorium/ds_item_spec.rb b/spec/digital_scriptorium/ds_item_spec.rb index 2247b3f..6d9fc1e 100644 --- a/spec/digital_scriptorium/ds_item_spec.rb +++ b/spec/digital_scriptorium/ds_item_spec.rb @@ -3,32 +3,24 @@ require 'wikibase_representable' module DigitalScriptorium + include ItemId include WikibaseRepresentable::Representers RSpec.describe DsItem do - let(:holding_json) { read_fixture('items/Q542_holding_example.json') } - let(:manuscript_json) { read_fixture('items/Q543_manuscript_example.json') } - let(:record_json) { read_fixture('items/Q544_record_example.json') } + let(:holding) { item_from_fixture('items/Q542_holding_example.json') } + let(:manuscript) { item_from_fixture('items/Q543_manuscript_example.json') } + let(:record) { item_from_fixture('items/Q544_record_example.json') } - it 'correctly reports if it is a holding' do - item = ItemRepresenter.new(described_class.new).from_json(holding_json) - expect(item.holding?).to be true + it 'correctly reports holding instance-of item ID' do + expect(holding.instance_of).to eq HOLDING end - it 'correctly reports if it is a manuscript' do - item = ItemRepresenter.new(described_class.new).from_json(manuscript_json) - expect(item.manuscript?).to be true + it 'correctly reports manuscript instance-of item ID' do + expect(manuscript.instance_of).to eq MANUSCRIPT end - it 'correctly reports if it is a record' do - item = ItemRepresenter.new(described_class.new).from_json(record_json) - expect(item.record?).to be true - end - - it 'returns instance_of (P16) claims' do - item = ItemRepresenter.new(described_class.new).from_json(record_json) - expect(item.instance_of_claims).not_to be_empty - expect(item.instance_of_claims.first.entity_id_value).to eq 'Q3' + it 'correctly reports record instance-of item ID' do + expect(record.instance_of).to eq DS_20_RECORD end end end diff --git a/spec/digital_scriptorium/ds_meta_spec.rb b/spec/digital_scriptorium/ds_meta_spec.rb index e298a0f..0c4e0d1 100644 --- a/spec/digital_scriptorium/ds_meta_spec.rb +++ b/spec/digital_scriptorium/ds_meta_spec.rb @@ -6,14 +6,11 @@ module DigitalScriptorium include WikibaseRepresentable::Representers RSpec.describe DsMeta do - let(:holding) { item_from_fixture('items/Q542_holding_example.json') } - let(:manuscript) { item_from_fixture('items/Q543_manuscript_example.json') } - let(:record) { item_from_fixture('items/Q544_record_example.json') } - context 'with a record concerning a manuscript with a single current holding' do it 'correctly sets the holding' do + record = export_hash.fetch('Q544') meta = described_class.new(record, export_hash) - expect(meta.holding).to eq holding + expect(meta.holding).to eq export_hash.fetch('Q542') end end diff --git a/spec/digital_scriptorium/export_representer_spec.rb b/spec/digital_scriptorium/export_representer_spec.rb index e33bb60..37566ed 100644 --- a/spec/digital_scriptorium/export_representer_spec.rb +++ b/spec/digital_scriptorium/export_representer_spec.rb @@ -1,34 +1,27 @@ # frozen_string_literal: true -require 'wikibase_representable' - module DigitalScriptorium include WikibaseRepresentable::Model - include WikibaseRepresentable::Representers RSpec.describe ExportRepresenter do - let(:holding_json) { read_fixture('items/Q542_holding_example.json') } - let(:manuscript_json) { read_fixture('items/Q543_manuscript_example.json') } - let(:record_json) { read_fixture('items/Q544_record_example.json') } - let(:property_json) { read_fixture('properties/P16_instance_of.json') } - let(:export) do - described_class.new(Export.new).from_json("[#{holding_json},#{manuscript_json},#{record_json},#{property_json}]") + it 'deserializes holding as a Holding' do + expect(export_hash.fetch('Q542')).to be_instance_of Holding end - it 'deserializes holding as a DsItem' do - expect(export[0]).to be_instance_of DsItem + it 'deserializes manuscript as a Manuscript' do + expect(export_hash.fetch('Q543')).to be_instance_of Manuscript end - it 'deserializes manuscript as a DsItem' do - expect(export[1]).to be_instance_of DsItem + it 'deserializes record as a Record' do + expect(export_hash.fetch('Q544')).to be_instance_of Record end - it 'deserializes record as a DsItem' do - expect(export[2]).to be_instance_of DsItem + it 'deserializes property as a Property' do + expect(export_hash.fetch('P16')).to be_instance_of Property end - it 'deserializes property as a Property' do - expect(export[3]).to be_instance_of Property + it 'deserializes non-core item as a DsItem' do + expect(export_hash.fetch('Q129')).to be_instance_of DsItem end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 6a5c687..e134227 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -16,10 +16,6 @@ end end -def property_config - YAML.load_file(File.expand_path('../property_config.yml', __dir__)) -end - def fixture_path(file) File.join(__dir__, 'fixtures', file) end @@ -43,31 +39,21 @@ def property_from_fixture(file) WikibaseRepresentable::Representers::PropertyRepresenter.new(property).from_json(read_fixture(file)) end -EXPORT_HASH = { - 'Q4' => item_from_fixture('items/Q4_holding_status_current.json'), - 'Q5' => item_from_fixture('items/Q5_holding_status_non_current.json'), - 'Q15' => item_from_fixture('items/Q15_non_dated.json'), - 'Q18' => item_from_fixture('items/Q18_author.json'), - 'Q21' => item_from_fixture('items/Q21_former_owner.json'), - 'Q33' => item_from_fixture('items/Q33_parchment.json'), - 'Q96' => item_from_fixture('items/Q96_14th_century.json'), - 'Q113' => item_from_fixture('items/Q113_latin.json'), - 'Q128' => item_from_fixture('items/Q128_provence.json'), - 'Q129' => item_from_fixture('items/Q129_spain.json'), - 'Q169' => item_from_fixture('items/Q169_cosmology.json'), - 'Q283' => item_from_fixture('items/Q283_deeds.json'), - 'Q300' => item_from_fixture('items/Q300_early_works.json'), - 'Q354' => item_from_fixture('items/Q354_almagest.json'), - 'Q374' => item_from_fixture('items/Q374_upenn.json'), - 'Q383' => item_from_fixture('items/Q383_schoenberg.json'), - 'Q394' => item_from_fixture('items/Q394_dioscorides.json'), - 'Q542' => item_from_fixture('items/Q542_holding_example.json'), - 'Q543' => item_from_fixture('items/Q543_manuscript_example.json'), - 'Q544' => item_from_fixture('items/Q544_record_example.json'), - 'Q1105' => item_from_fixture('items/Q1105_deste.json'), - 'Q1106' => item_from_fixture('items/Q1106_llangattock.json') -}.freeze - +# Return a fixture export hash for all item and property JSON files in +# +fixtures/items+ and +fixtures/properties+. Returned fixture has Manuscript, +# Holding, and Record items, as well as corresponding Items for authority +# values: term, name, place, material, etc. +# +# @return [Hash] def export_hash - EXPORT_HASH + @export_hash ||= begin + fixtures = [] + %w[items properties].each do |fixture_folder| + fixtures << Dir[File.join(__dir__, 'fixtures', fixture_folder, '*.json')].map do |file| + File.read(file) + end + end + export_json = "[#{fixtures.join(',')}]" + DigitalScriptorium::ExportRepresenter.new(DigitalScriptorium::Export.new).from_json(export_json).to_hash + end end