Re: [Gen-art] Genart last call review of draft-ietf-cbor-cddl-05

[Alissa Cooper <[e-mail]>]

Ines, thanks for your review. Carsten, thanks for addressing the review comments. I have entered a No Objection ballot.

Alissa

> On Nov 6, 2018, at 6:51 PM, Carsten Bormann <[e-mail]> wrote:
> 
> Hi Ines,
> 
> Thank you very much for the review and the slight embarrassment it causes to the authors…
> 
> Comment 3 below we will pick up in the WG meeting today; I’ll report about that later.
> The other comments are now addressed in the editor’s copy:
> 
> https://github.com/cbor-wg/cddl/commits/master
> (Individual commits referenced below; the CI build is failing right now because of a weird connectivity issue.)
> 
> We plan to submit the fixes as part of -06 early next week.
> 
> Grüße, Carsten
> 
> 
> 
>> On Sep 29, 2018, at 05:02, Ines Robles <[e-mail]> wrote:
>> 
>> Reviewer: Ines Robles
>> Review result: Ready with Nits
>> 
>> I am the assigned Gen-ART reviewer for this draft. The General Area
>> Review Team (Gen-ART) reviews all IETF documents being processed
>> by the IESG for the IETF Chair.  Please treat these comments just
>> like any other last call comments.
>> 
>> For more information, please see the FAQ at
>> 
>> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
>> 
>> Document: draft-ietf-cbor-cddl-05
>> Reviewer: Ines Robles
>> Review Date: 2018-09-28
>> IETF LC End Date: 2018-10-04
>> IESG Telechat date: Not scheduled for a telechat
>> 
>> Summary:
>> 
>> I believe the draft is technically good. This document is well written and
>> clear to understand.
>> 
>> The document proposes a notational convention named Concise data definition
>> language (CDDL) to express Concise Binary Object Representation (CBOR) data
>> structures. Its main goal is to provide an easy and unambiguous way to express
>> structures for protocol messages and data formats that use CBOR or JSON.
>> 
>> I have some minor questions. The document presents nits that should be solved
>> before publication.
>> 
>> Major issues: Not found.
>> 
>> Minor issues: Not found.
>> 
>> Nits/editorial comments:
>> 
>> 1)-Abstract: It would be nice to expand CBOR the first time that it is
>> mentioned.
> 
> Indeed.  Fixed in 88e9a00
> 
>> 2)-Section 2:  Page 8,  You mention anonymous group.
>> Does it would be a good definition for an anonymous group?: “Lists of
>> group_entries_  (between curly braces)that can be assigned to any type of
>> composition (arrays, maps)”
> 
> The groups are anonymous because they are used right away in a pair of curly braces (for a map) or square brackets (for an array).
> The examples in this section all happen to be in braces.
> I’m not sure that “anonymous group” needs to be defined as a separate term, though, because it is just that: a group that is anonymous.
> 
>> 3)- Section 2.2.2.1 - Page 11: the ordering I think is ascendant, right? So,
>> maybe, would it be nice to add “natural ordering relationship”? question: is
>> the following valid?:
>> 
>>       Take-off = {
>>               takeoffcounting= 10..0 ; Is it valid? Is it applicable?
>> }
> 
> The type constituted by the range does not have any ordering relationship (except that implied by numbering in general).
> The example you give would not make a difference from saying 0..10 — in both cases, the eleven numbers 0 to 10 constitute the type.
> 
> It is probably not so good that the current text seems to leave open whether 10..0 means the same as 0..10.
> 
> (There is no obvious right answer, so we will discuss this in the CBOR WG meeting today and come back.)
> 
>> 4)- In this paragraph: “...When using a name as the left hand side of a range
>> operator, use spacing as in
>>  "min .. max" to separate off the range operator...."
>> => Question: Is “min .. max" equivalent to "min” .. “max"?
> 
> Sorry for leading the reader into quoting hell.
> The quotes here are generated by XML2RFC to delimit a code span, i.e., they identify the example.
> Maybe we should make small figures out of these (ignoring that some people don’t like sentences running over figures).
> 
> Fixed in a530564
> 
>> 
>> 5)- There are some mismatches between Appendix B and Appendix C:
>> 
>>       Appendix B: cddl = S 1*(rule S)
>>       Appendix C: cddl = S 1*rule
>> ------------------------------------------------------------
>> 
>>       Appendix B:
>> rule = typename [genericparm] S assignt S type
>>        / groupname [genericparm] S assigng S grpent
>> 
>>       Appendix C:
>> rule = typename [genericparm] S assignt S type S
>>       / groupname [genericparm] S assigng S grpent S
>> ------------------------------------------------------------
>> 
>>       Appendix B:    type = type1 *(S "/" S type1)
>>       Appendix C:    type = type1 S *("/" S type1 S)
>> 
>> ------------------------------------------------------------
>>       Appendix B:   group = grpchoice *(S "//" S grpchoice)
>>       Appendix C:   group = grpchoice S *("//" S grpchoice S)
>> 
>> ------------------------------------------------------------
>>       Appendix B:    grpchoice = *(grpent optcom)
>>       Appendix C:    grpchoice = *grpent
>> 
>> ------------------------------------------------------------
>> 
>>       Appendix B:    grpent = [occur S] [memberkey S] type
>>       Appendix C:    grpent = [occur S] [memberkey S] type optcom
>> 
>> ------------------------------------------------------------
>> 
>>       Appendix B:    / [occur S] groupname [genericarg]  ; preempted by above
>>       Appendix C:    / [occur S] groupname [genericarg] optcom ; preempted by
>>       above
>> 
>> ------------------------------------------------------------
>> 
>>       Appendix B:   / [occur S] "(" S group S ")"
>>       Appendix C:   / [occur S] “(" S group S ")" optcom
> 
> This is the embarrassing part.  We recently cleaned up the whitespace processing in the ABNF a bit, and I was sure all the changes made it to the matching appendix.  D’oh.  Another case of “a feature isn’t there if it isn’t checked by continuous integration checking”.
> 
> Fixed in bfdecd8
> 
>> 
>> 6)- It would be nice to expand I-JSON to (“Internet JSON") in page 54
> 
> Fixed in cc43317
> 
> Grüße, Carsten
> 
> _______________________________________________
> Gen-art mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/gen-art