Hello Joe, Thank you for your review and apologies for the delay! Please find our answers to your questions below. The diff with -15 is available here [1]. The updated version is available as txt [2] and as html [3]. Thanks, Ivaylo [1]: https://tools.ietf.org/rfcdiff?url1=draft-ietf-core-yang-cbor&url2=http://core-wg.github.io/yang-cbor/draft-ietf-core-yang-cbor-latest.txt [2]: https://core-wg.github.io/yang-cbor/draft-ietf-core-yang-cbor-latest.txt [3]: https://core-wg.github.io/yang-cbor/draft-ietf-core-yang-cbor-latest.html On Tue, Mar 16, 2021 at 8:37 PM Joe Clarke via Datatracker <noreply@xxxxxxxx> wrote: > > Reviewer: Joe Clarke > Review result: Almost Ready > > I have been asked to review this document on behalf of yang-doctors. Overall, > I found the document with its many examples to be quite readable and clear. > However, I did find a few readable and typo issues and I have a few questions. > See below. > > === > > Abstract: > > s/notifications and yang-data/notifications and the yang-data/ [IP]: Fixed. > === > > Section 2: > > Move the definition of YANG Schema iDentifier higher up in the list of > terminology so it's defined before you use it when discussing deltas. This > should likely be first in the list. [IP]: Fixed. > === > > Section 3 > > s/When schema node are serialized/When schema nodes are serialized/ [IP]: Fixed. > === > > Section 3.3 > > s/identifiers as string, similar/identifiers as strings, similar/ [IP]: Fixed. > === > > Section 4.1 > > In other examples you include the associated type definition inline. You don't > do that with inet:domain-name. In fact, you _do_ include the domain-name > typedef in Section 4.3, but I think it should move up here as well to aid in > readability. [IP]: Added the typedef here as well to make the example self-contained and simpler for reading. > === > > Section 4.2.1 > > s/In the case of an 'notification content'/In the case of a 'notification > content'/ [IP]: Fixed. > === > > Section 4.2.2 > > You duplicate the YANG snippet here that you included in the overarching > Section 4.2. I don't think both are needed. Probably best to drop this here > since you didn't include it in 4.2.1. [IP]: Agreed! Fixed. > === > > Section 4.4 > > You use the term "list instance" to mean what I think is better stated as "list > entry". The latter is clearer with respect to an element within a list vs. the > instance of a list itself. You use "list instance" in Section 3 as well (and > in other places in the document) where I think "list entry" would be clearer. [IP]: There were places where instances were used as instantiations of schema nodes and places where instances were used instead of entries. I tried to clear this out. > === > > Section 4.4.1 > > I think documenting the true/false value of the primitives here (noted in the > CBOR encoding) would aid in clarity. For example, "primitive(20) [false]". [IP]: I am not against that, I am only concerned if that would be readable for others that are used to the diagnostic notation, otherwise I am fine to apply this change. > === > > Section 4.5.1 > > I'm being pedantic here, but ahead of the { 60123 : { ... example, you usually > state "CBOR diagnostic output". You don't here, though. I think you should > add it. [IP]: I might be misunderstanding the point, but it appears to me that there is such a note already, only it unfortunately appeared at the top of the previous page in the txt version and was quite easy to miss. > === > > Section 4.6 > > Concerning text "anyxml value MAY contain CBOR data items tagged with one of > the tags listed in Section 9.3, these tags shall be supported.": > > This sentence fragment makes no sense. Did you mean something along the lines > of: "the tags listed in Section 9.3 SHALL be supported"? [IP]: I believe I fixed that now. > === > > Section 5 > > s/Just like YANG containers, yang-data/Just like YANG containers, the yang-data/ [IP]: Fixed. > === > > Section 6.7 > > s/same example yang definition, but this/same example YANG definition, but this/ [IP]: Fixed. > s/byte string MUST instead by encoded/byte string MUST instead be encoded/ [IP]: Fixed. > === > > Section 6.13.1 > > It isn't clear to me how a YANG list with multiple keys or a YANG list with no > keys would be encoded in CBOR. I think examples and some clarifying text would > help. [IP]: I have modified one of the examples so that it uses two keys. As for the other point, Is it possible to have a list with no keys being referenced through an instance-identifier? My understanding is that this is not possible, but I might be wrong. If it is not possible, we will only need to clarify this in the text. If it is possible, can we use the position in the list to identify the element? > > > _______________________________________________ > core mailing list > core@xxxxxxxx > https://www.ietf.org/mailman/listinfo/core -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
