-
Notifications
You must be signed in to change notification settings - Fork 63
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
restrict more uri variables #1109
Conversation
Thanks for fixing the weird numbers that were popping up as well :) I think that the text needs a tiny bit of polishing:
It is not exactly clear that the keys in the uriVariables map cannot be of ObjectSchema or ArraySchema, i.e. it reads as if uriVariables cannot be an object, it always is. Not sure but maybe something like: -> Define URI query template variables as a collection based on DataSchema declarations. The individual variables' (keys') DataSchema cannot be an ObjectSchema or an ArraySchema. There was also a missing |
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.
Nicely done, except for @egekorkan's comment the PR looks good. Just a side note, in #1068 I mentioned the possibility to merge uriVariables
in the input
object for actions avoiding ambiguities. It is fine that it is not addressed in this PR, but I would open a dedicated issue so that we can discuss this possibility. Maybe it could even be deferred for 2.0. What do you think?
<p> | ||
<code>uriVariables</code> are mainly for properties and events. When retrofitting an existing system, it may be necessary to use | ||
<code>uriVariables</code> for actions. In general, it is recommended to avoid <code>uriVariables</code> as much as possbile when a | ||
new WoT-based system is designed. |
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 for making this an explicit statement of the specs.
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.
year, that would be a kind of bigger change and should be more addressed in the TD 2.0 version. I would suggest that you create a new issue and I label it with defer 2.0
new commit includes the CR from Ege |
from today's TD call it was decided to merge this PR |
also see #1068
Preview | Diff