samvera / hydra-pcdm

Samvera implementation of the PCDM model
Other
11 stars 10 forks source link

Rename #collections to child_collections #109

Closed jcoyne closed 9 years ago

jcoyne commented 9 years ago

Collection#collections used to return collections that contained this object (parent collections, see https://github.com/projecthydra/hydra-collections/blob/master/lib/hydra/collections/collectible.rb#L10 ) but now collections returns child collections. This renaming hopes to reverse the incompatible naming change.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.17%) to 96.89% when pulling b43850305fc09dc52970e029e96b289581d6f55f on rename_collection into d9c5b7f1d268638cfcd809052c65eb82210419db on master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.03%) to 97.09% when pulling 38e0f85c1e1313d07abd67d575813f916d7e3536 on rename_collection into d9c5b7f1d268638cfcd809052c65eb82210419db on master.

elrayle commented 9 years ago

I thought about this more overnight. I would propose the rename of #collections, which currently returns all child collections of a collection, be renamed to #child_collections. This is inline with the parent-child terminology used in the services and will work better at all levels (e.g. #child_generic_works is more intuitive than #subgeneric_works).

elrayle commented 9 years ago

To be consistent, similar changes need to be made at all levels of the model in pcdm and hydra works.

jcoyne commented 9 years ago

@elrayle they are now child_collections. I can update the other gems, once this PR is merged.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.03%) to 97.09% when pulling 235312321c1d9c8ad887796c6f60597645e7d103 on rename_collection into d9c5b7f1d268638cfcd809052c65eb82210419db on master.

elrayle commented 9 years ago

I like that the solr tests were moved out of the service tests.

I don't work with solr much, so the following are observations and questions to help me understand that this is setup the way we want.

Indexer Classes

ObjectIndexer

NOTE: objects_ssim should become child_objects_ssim and #objects should become #child_objects.

CollectionIndexer < ObjectIndex

What gets (should get) indexed?

The parent ids aren't indexed. Are these relationships inferred from the other indexed ids? With the way this is written, can you retrieve the all parents of an object that lives in multiple collections? Or is this work still in progress?

jcoyne commented 9 years ago

We do not want to index parent collections. We can infer the relationship by doing a solr join and it's challenging to find parents just by looking at LDP.