Thank you Matthias for the thorough review! I have prepared a PR to accommodate your comments: https://github.com/core-wg/senml-etch/pull/9/files Please see also below comments and discussion on each issue. > On 2 Sep 2019, at 16.08, Matthias Kovatsch via Datatracker <noreply@xxxxxxxx> wrote: > > Reviewer: Matthias Kovatsch > Review result: Ready with Nits [...] > It would be good to get the help from an expert on Windows Clipboard Formats > and Macintosh Uniform Type Identifiers, as no good guidelines are available to > check these IANA considerations. (This issue appears to be recurrent also for > other specs.) I agree; I used here the same pattern we used for the SenML RFC (which I think we copied from some older RFC or W3C spec), but indeed better guidance in general would be useful. > ## Technical comments > > * P4 (3.1): I am missing assertions such as "Values in a Fetch Record MUST be > ignored." Actually this applies to all fields other than (base)name and time. I added a note to end of 3.1: "All other Fetch Record fields than name, base name, time, and base time MUST be ignored." I also noted this difference to SenML (lack of values) in the intro. > * What should happen when a Patch Record does not have a value? Good catch! There should be either value or sum field in all Patch records; otherwise the resulting record would not be valid SenML record. I added following to 3.2: "All Patch Records MUST contain at least a SenML Value or a Sum field. A Patch Pack with invalid Records MUST be rejected." I noticed also that the first sentence of 3.2 was a bit misleading; I changed that from "The (i)PATCH method can be used to change the values of SenML Records" to "[...] change the fields of SenML Records". > * P5 §3: The record must not be added when the value is null. (behavior not > described formally enough) Changed this: OLD: If a Patch Record has a value ("v") field with value null, the matched Record (if any) is removed from the Pack. NEW: If a Patch Record has a value ("v") field with value null, it MUST NOT be added but the matched Record (if any) is removed from the Target Pack. I also added definition of Target Pack: "A SenML Pack that is a target for a Fetch or Patch operation." > * P7: "Windows Clipboard Name" --> Microsoft and for instance HTML spec use > "Windows Clipboard Format" > * Okay, the sting itself is the Windows Clipboard Format Name... That naming convention was used also by W3C specs (and now SenML RFC) with media type registrations. > * The long string with spaces ("SenML FETCH/PATCH format") is a bit weird for > this purpose, no? Spaces seem to be OK here: for example SVG Image has clipboard format name "SVG Image". But if there's better guidance available, I'd be happy to follow that. > * I also had the problem to find proper guidelines for Windows Clipboard > Formats; are there any? I'm not aware of any and didn't find any good sources based on quick googling. > * No Macintosh Uniform Type Identifier? There are actually Mac UTIs in sections 6.2 and 6.3. > ## Additional comment > > * As already discussed with one of the authors, an implication for LwM2M is > probably that these patch documents must not be used with Executable Resources > (one might try to execute multiple resources at once with a PATCH method). The > application of a Patch Pack is then not idempotent anymore. Furthermore, it is > unclear what the value should be when the LwM2M Executable Resource does not > take arguments. * If executing multiple resources atomically is an important > use case, I think we need another iteration to deal with the state vs RPC issue > ("use PATCH to call function(s) without arguments by giving a new state?!") Indeed. I'm not aware of use case for using Patch with LwM2M exec resources, but can double check this. > ## Editorial comments > > * P1 §1 (Abstract), P2 last §: "iPATCH, PATCH, and FETCH" --> "FETCH, PATCH, > and iPATCH" > * It is easier on the brain if the order is kept consistent... Fixed. > * P2 §6: "hence full name" --> "hence the unique identifiers" ? > * RFC 8428 does not define or contain "full name", but "globally unique > identifier for the resource" Good point. 8428 also uses simply "name" to refer to the "unique ID that results in the concatenation of the name and base name fields". I think using "name" is less confusing in this context so I would suggest changing "full name" to just "name". But would be great to hear views of others on this too. > * P3 §1: "The semantics of the ..." > * Creates question about semantics for FETCH > * Better to reverse sentences and start with "The rest of the document uses > the term "(i)PATCH" to refer to both methods, as the semantics of the new > media types are the same for the CoAP PATCH and iPATCH methods." Fixed. > * P3 §1: ", that can be used with the" --> ", which ..." Fixed. > * P3 §3: "Also the following ..." --> to many "also", just "The following ..." Changed to "The following additional terms" to make it clear this adds (and does not contradict) to the previous terms. > * P4 §2 (3.1): "... when resolved, match resolved names" --> "identifiers" > * names when resolved are resolved names, hence unclear what is compared > * P2 calls them "full names" > * See above, should be something like "globally unique identifier for the > resource" To make this more clear I'd suggest adding "..match resolved names in a *Target* SenML Pack". Alternatively we could go for the "(globally) unique IDs" way. > * P4 §8: Add example for records with name and time Added. > * Would be good to quickly show what "resolved form of records" means I added a short example of this to the end of the SenML FETCH section: "In resolved form the SenML name in the example above becomes '2001:db8::2/3311/0/5850'. Since there is no base time in the Pack, the time in resolved form is equal to the time in the example." > * P4 §9 (3.2): Add statement that SenML patch documents are always idempotent, > hence PATCH and iPATCH are equivalent? > * Basically move the last sentence to the beginning and give explanation for > "(i)PATCH". IMHO, moving the last sentence to the beginning would make the flow of the text a bit weird. But I suggest changing the last sentence of the paragraph into: "Application of Patch Packs is idempotent; hence PATCH and iPATCH methods for SenML Packs are equivalent." > * P5 §2: "When the name" --> "When the resolved name" ? Fixed. Thank you once more for the review! Cheers, Ari
