-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
V3 canvas: improve specificity, validation and tests #25
Conversation
|
||
def required_keys | ||
super + %w{ id label } | ||
end | ||
|
||
def array_only_keys | ||
super + %w{ content } | ||
def prohibited_keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why you made the changes to remove int_only_keys and the additions to prohibited_keys and legal_viewing_hint_values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great question! So the int_only_keys
that were here in Canvas are now in abstract_resource
; prohibited_keys
is a new thing (partly because so many keys are in abstract_resource
and partly because the v3.0 spec is full of "this key MUST not be used elsewhere" language); legal_viewing_hint_values are potentially unique to different classes so they need to be specified at this level, and I used the values indicated by the current v3.0 draft.
It's possible that the placement of stuff in abstract_resource
vs. individual classes still could use tweaking ... I think when Chris added the int_only_keys
and a few other such methods, he included all or nearly all the keys in the abstract_resource
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This is mergable. In working on this, I spawned these purl tickets: