-
Notifications
You must be signed in to change notification settings - Fork 4
readallproperties
operation - closes #51
#77
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
base: master
Are you sure you want to change the base?
Conversation
b8b53b4
to
9f17f5d
Compare
9f17f5d
to
65bdfb1
Compare
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.
This all looks good with only a single exception.
I recommend the use of 'value' instead of 'values' as the response value, as it holds well ... the response value. That the content in this case is an object with property values doesn't neccesitate the change of the name from value to values. Instead using the same name for all response types simplifies implementation as the there is no need to look for different property names different operations.
</td> | ||
</tr> | ||
<tr> | ||
<td><code>values</code></td> |
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.
I recommend to use the name 'value' to contain the response value instead of 'values'.
- This is consistent with readproperty.
- The caller is requesting readallproperties. The result is a value that holds an object. Nothing is gained by using a different name. Instead the developer has to check the operation to know the result can be found under a different property name.
- Implementation is easier using 'value' as the value of the response can always be found in the property 'value'.
Thank you for the review @hspaay.
The Any code implementing this operation would have to have different logic for the different operations and treat My personal opinion is therefore that it's important to give it a different name to signal that they are different and that in the case of Of course if I'm alone in this view I am open to the group consensus.
I definitely don't agree that the payloads of all operations should have a generic member called "value" if that's what you mean. As you know, in my proposal in #42 (which is a middle ground between the original strawman proposal and your suggestion for a single payload format for all messages) I still use the terms Whilst I understand the OOP instinct to abstract similar looking classes, experience tells me too much abstraction can be harmful and actually I think clarity is more important than consistency in this case. I don't really believe it would necessarily simplify implementation either because the logic has to be different regardless. That's really a separate topic but as always I'm open to other views if the consensus leads elsewhere. |
@benfrancis Yes I know we are on a different page with this one. Understandable as you view the message as the operation while I view it as a generic envelope. Either way it will be able to do its thing so its all good :) Whatever the end decision is I'll update hiveot accordingly. |
Closes #51.
This PR defines request and response message formats for a readallproperties operation.
There was some discussion in #27 about whether all the property readings should be contained in one message or whether they should all come in separate messages.
Having given this some thought my personal preference is for all of the property readings to be contained in a single message, for the following reasons:
observeallproperties
operation, where each property reading will arrive in a separate message when it changes. We could potentially also have that operation report the current value of all properties in individual messages when the observation is first started which would satisfy that use case.Let me know what you think.
Preview | Diff