Skip to main content

Last Call Review of draft-ietf-core-yang-cbor-15
review-ietf-core-yang-cbor-15-yangdoctors-lc-clarke-2021-03-16-00

Request Review of draft-ietf-core-yang-cbor-15
Requested revision 15 (document currently at 20)
Type Last Call Review
Team YANG Doctors (yangdoctors)
Deadline 2021-03-17
Requested 2021-02-24
Requested by Marco Tiloca
Authors Michel Veillette , Ivaylo Petrov , Alexander Pelov , Carsten Bormann , Michael Richardson
I-D last updated 2021-03-16
Completed reviews Yangdoctors Last Call review of -15 by Joe Clarke (diff)
Secdir Last Call review of -15 by Shawn M Emery (diff)
Genart Last Call review of -15 by Peter E. Yee (diff)
Genart Telechat review of -16 by Peter E. Yee (diff)
Assignment Reviewer Joe Clarke
State Completed
Request Last Call review on draft-ietf-core-yang-cbor by YANG Doctors Assigned
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/rSbM6sfldDQ7aisvB9YeY0pCuNE
Reviewed revision 15 (document currently at 20)
Result Almost ready
Completed 2021-03-16
review-ietf-core-yang-cbor-15-yangdoctors-lc-clarke-2021-03-16-00
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/

===

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.

===

Section 3

s/When schema node are serialized/When schema nodes are serialized/

===

Section 3.3

s/identifiers as string, similar/identifiers as strings, similar/

===

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.

===

Section 4.2.1

s/In the case of an 'notification content'/In the case of a 'notification
content'/

===

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.

===

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.

===

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]".

===

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.

===

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"?

===

Section 5

s/Just like YANG containers, yang-data/Just like YANG containers, the yang-data/

===

Section 6.7

s/same example yang definition, but this/same example YANG definition, but this/

s/byte string MUST instead by encoded/byte string MUST instead be encoded/

===

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.