Use class methods for Valkyrie model duck‐typing #6421
Conversation
why deprecate and not just keep them? |
i think Admin Sets are ambiguous at the abstract model layer. initially, they were introduced without reference to any PCDM or Hydra::Works concepts. the Collections Extensions work effort later tried (incompletely) to wrap them into the Collection Types concept; so an Admin Set would be "just" a Collection with a certain Collection Type. i can't see any downsides to treating AdministrativeSet instances as |
no-reply
force-pushed
the
valkyrie_duck
branch
from
4d04d6c
to
304bbe0
Compare
the (extremely slight) conceptual convenience to treating them separately is that they use different forms. but it’s easy enough to just check if something is an admin set first, prior to checking if it is a PCDM collection |
Honestly I was going back and forth about this; it seemed kind of weird to have both (considering that it would be unexpected if they disagreed), but the convenience might outweigh those concerns. They can’t be removed in the near term regardless, so I guess it just depends whether removing them in the long term is desirable or not. (If so, it seems better to start issuing deprecation warnings as soon as possible.) |
marrus-sh
force-pushed
the
valkyrie_duck
branch
2 times, most recently
from
5b873ec
to
00d6c2a
Compare
|
@no-reply just pushed two additional commits:
|
It may be necessary to determine whether a model *class* represents a PCDM Object, File Set, etc, not just a model *instance*. This commit defines the former and aliases and deprecates the latter. `collection?` and `pcdm_collection?` are split, in that `Hyrax::AdministrativeSet` and `Hyrax::PcdmCollection` are both the former, but not both the latter. Is this right, or should those methods be synonyms? `work?` is an alias for `pcdm_object? & !file_set?`. `Hyrax::PcdmCollection` used to think it was a `pcdm_object?`, but it shouldn’t have. There’s probably a fair amount of `model.try(:work?)` which needs to be changed to `model.class.try(:work?) || model.try(:work?)` before the old methods can be removed. We’ll need to support both for as long as we support ActiveFedora, but normalizing on a single place for querying this information, and having that place be the model class, seemed desirable for Valkyrie resources. Notably, this enables the use of these methods for determining which form or indexer to use for a class of resource, instead of relying on class inheritance checks.
In discussion, it sounded like deprecating these wasn’t worth it. Instead, add tests to the `Hyrax::Resource` shared examples to affirm that the instance and class methods match for all of these values.
It seems like there has been an attempt to consider administrative sets as kinds of collections in the PCDM model, and we should push ahead with that going forward. This means that `:collection?` and `:pcdm_collection?` are effectively synonyms. I prefer the latter (because it mirrors `:pcdm_object?`), but it’s new and most Hyrax code currently uses the former.
marrus-sh
force-pushed
the
valkyrie_duck
branch
from
12ac4be
to
d2e0d54
Compare
It may be necessary to determine whether a model class represents a PCDM Object, File Set, etc, not just a model instance. This commit defines the former and aliases
and deprecatesthe latter.collection?andpcdm_collection?are split, in thatHyrax::AdministrativeSetandHyrax::PcdmCollectionare both the former, but not both the latter. Is this right, or should those methods be synonyms?work?is an alias forpcdm_object? & !file_set?.Hyrax::PcdmCollectionused to think it was apcdm_object?, but it shouldn’t have.There’s probably a fair amount ofFor the time being, we’ll just support both.model.try(:work?)which needs to be changed tomodel.class.try(:work?) || model.try(:work?)before the old methods can be removed. We’ll need to support both for as long as we support ActiveFedora, but normalizing on a single place for querying this information, and having that place be the model class, seemed desirable for Valkyrie resources.Notably, this enables the use of these methods for determining which form or indexer to use for a class of resource, instead of relying on class inheritance checks.