Add Set_HWD and related tests #70
Conversation
iiif_prezi3/helpers/set_hwd.py
Outdated
| if duration is None and height is None and width is None: | ||
| raise TypeError("At least one of height, width, or duration must be set") | ||
| if height is not None and width is None: | ||
| raise TypeError("width must be set if height is set") | ||
| if width is not None and height is None: | ||
| raise TypeError("height must be set if width is set") |
There was a problem hiding this comment.
Is zero allowed, or any other empty values? Or is important to check explicitly for None?
If not, could just do a boolean check here, e.g. if not (height or width or duration)
There was a problem hiding this comment.
The spec doesn't explicitly say width and height must be non-zero, but then it also doesn't say they must be positive integers either.
There was a problem hiding this comment.
I suppose there could be an instance where a user tries to set H+W to 0 for an Audio resource, which would technically be correct, I suppose?
There was a problem hiding this comment.
Mmm interesting point I think both solutions are valid, maybe Rebecca choice is safer if one wants to enforce h and w to be zero he could assign them manually
There was a problem hiding this comment.
The spec says "The value must be a positive integer." so I think it implies its 1 or over...
There was a problem hiding this comment.
@glenrobson @rlskoeser @digitaldogsbody In the json schema however the minimum for duration is 0:
https://github.com/IIIF/presentation-validator/blob/bd7468500f8ebfa82aa54f6c80ddd4f22f20c71a/schema/iiif_3_0.json#L62
There was a problem hiding this comment.
The spec says "The value must be a positive integer." so I think it implies its 1 or over...
Somehow my eyes completely bypassed that sentence when I was reading the spec 😂
Is there a way to enforce this in the Schema? That would seem the best place to do so, but if it's not possible, then we should add it here in the helper.
There was a problem hiding this comment.
@digitaldogsbody i think that "minimum": 1 should do the trick!
https://github.com/IIIF/presentation-validator/blob/bd7468500f8ebfa82aa54f6c80ddd4f22f20c71a/schema/iiif_3_0.json#L62
There was a problem hiding this comment.
We'd also need to add a minimum for the height and width:
https://github.com/IIIF/presentation-validator/blob/bd7468500f8ebfa82aa54f6c80ddd4f22f20c71a/schema/iiif_3_0.json#L141
https://github.com/IIIF/presentation-validator/blob/bd7468500f8ebfa82aa54f6c80ddd4f22f20c71a/schema/iiif_3_0.json#L531
One thing we should check is if a minimum value of 1 will prevent blank values (because of an evaluation to 0). I don't know if this is the case with how Pydantic works (I suspect not) but it might be for some JSON Validators?
The potential issue is that for an audio file, H & W = 0 would technically be correct, although it would violate the wording of the spec (if we read "positive integer" as >= 1, that is), so we should maybe have a chat about how we want to handle this.
There was a problem hiding this comment.
@azaroth42 @zimeon @jpstroop - any thoughts on this as editors?
Co-authored-by: Rebecca Sutton Koeser <rlskoeser@users.noreply.github.com>
|
I have opend an issue #79 for the duration problem. |
Co-authored-by: Rebecca Sutton Koeser <rlskoeser@users.noreply.github.com>
Co-authored-by: Rebecca Sutton Koeser <rlskoeser@users.noreply.github.com>
|
if you are able to get this merged, I should have time tomorrow to update and finish my work on #56 |
|
I think we can merge without the minimum duration being resolved, since that will be a change in the schema and we still need to determine if 0 can be valid in some circumstances (per discussion above). |
This add pull request adds the method Set_HWD to Canvases and ResourceItem and tests for checking it works correctly.
#54