V3 abstract_resource beef up #19
Conversation
…fects sequence due to 'items')
ndushay
force-pushed
the
v3-abstract-resource-beef-up
branch
from
3a442bd to
2ebd24b
Compare
There was a problem hiding this comment.
left one question, and one nitpick (repeated, but same across instances). but 👍 overall.
the question is the one inline about the _PROPERTIES, possibly just me not understanding the usage.
re: the nitpick: my general preference is to not specify an exception class when expecting that an error isn't thrown, unless you know an error will be thrown, and you're just making sure it's not of a specific type. but it looks like the cases i pointed out are cases of valid usage, where you'd expect execution without error? anyway, not something that i feel super strongly about, and i'd merge as-is if you really prefer the current approach.
| end | ||
| it 'does not raise error for entry that contains exactly "label" and "value"' do | ||
| subject['metadata'] = [{ 'label' => 'bar', 'value' => 'foo' }] | ||
| expect { subject.validate }.not_to raise_error(IIIF::V3::Presentation::IllegalValueError) |
There was a problem hiding this comment.
what about just expect { subject.validate }.not_to raise_error?
There was a problem hiding this comment.
sure - that's fine by me. Will change
| end | ||
| it 'does not raise error for entry with "id" and "type"' do | ||
| subject['thumbnail'] = [{ 'id' => 'bar', 'type' => 'foo', 'random' => 'xxx' }] | ||
| expect { subject.validate }.not_to raise_error(IIIF::V3::Presentation::IllegalValueError) |
There was a problem hiding this comment.
ditto on not looking for a specific exception class
| describe 'nav_date' do | ||
| it 'does not raise error for value of form YYYY-MM-DDThh:mm:ssZ' do | ||
| subject['nav_date'] = '1991-01-02T13:04:27Z' | ||
| expect { subject.validate }.not_to raise_error(IIIF::V3::Presentation::IllegalValueError) |
There was a problem hiding this comment.
ditto on not looking for a specific exception class
| end | ||
| it 'does not raise error for entry with "id" that is URI' do | ||
| subject['rights'] = [{ 'id' => 'http://example.org/rights', 'format' => 'text/html' }] | ||
| expect { subject.validate }.not_to raise_error(IIIF::V3::Presentation::IllegalValueError) |
There was a problem hiding this comment.
ditto on not looking for a specific exception class
| end | ||
| it 'does not raise error for entry with "label" and "format"' do | ||
| subject['rendering'] = [{ 'label' => 'bar', 'format' => 'foo', 'random' => 'xxx' }] | ||
| expect { subject.validate }.not_to raise_error(IIIF::V3::Presentation::IllegalValueError) |
There was a problem hiding this comment.
ditto on not looking for a specific exception class
| CONTENT_RESOURCE_PROPERTIES = %w{ format height width duration } | ||
|
|
||
| # used by Collection, AnnotationCollection | ||
| PAGING_PROPERTIES = %w{ first last next prev total start_index } |
There was a problem hiding this comment.
nothing seems to use these new _PROPERTIES keys, though there is suggested usage below... are these something that will be used in the future? by consumers of the gem? something used by some meta-programming magic that i'm not seeing/understanding?
There was a problem hiding this comment.
these will be used by subclasses to make it a lot easier to specify prohibited_keys. The spec is full of language like "Other resource types must not have a xxx" and these are two groups of properties that are basically used together, or at least in the same way.
setting up
abstract_resourceto do more heavy lifting for keys/properties that are used by multiple subclassesI think this is simple enough that one reviewer is enough.