Subject: [yang-doctors] Yangdoctors last call review of draft-ietf-spring-sr-yang-20

From: Ladislav Lhotka via Datatracker <[email-masked]>
Date: Mon, 24 Aug 2020 05:51:30 -0700

Reviewer: Ladislav Lhotka
Review result: Ready with Nits

I also did an early YANG Doctors review [1]. My comments regarding YANG module
revisions and normative references are addressed in the current revision. The
suggested naming changes were either accepted or, I assume, addressed in the WG
and rejected (which is OK).

Compared to the previously reviewed revision -09, the current revision contains
one additional YANG module: ietf-segment-routing-mpls. This module adheres to
the same high standards as the previous two, and I discovered no issues with
all of them.

[1] https://datatracker.ietf.org/doc/review-ietf-spring-sr-yang-09-yangdoctors-early-lhotka-2018-10-24/

 Comments
------------

- The title of Section 6 (States) still looks weird to me. My suggestion is to
use "State Data" instead.

- The title of Section 8 should use plural "YANG Modules" because it contains
three modules. It would also be helpful to introduce a subsection for each
module.

- Due to the RFC line length limit, the example in Appendix A uses a line break
inside a URI of a XML namespace declaration, which makes the XML invalid. This
can be probably avoided by including the XML namespace declaration for "sr-cmn"
in the top-level element, i.e.

  <routing
    xmlns="urn:ietf:params:xml:ns:yang:ietf-routing"
    xmlns:sr-cmn="urn:ietf:params:xml:ns:yang:ietf-segment-routing-common">

  If not, it would be better to use conventions of RFC 8792.

- Assuming that the example is intended for human readers, it might be better
to provide it in the JSON representation per RFC 7951.

---

From: Yingzhen Qu <[email-masked]>
Date: Tue, 25 Aug 2020 20:16:12 +0000

Hi Lada,

Thank you for your review, really appreciate. 

I've uploaded version -21 to address your comments and please see detailed answers below inline starting with [YQ].

Thanks,
Yingzhen

On 8/24/20, 5:51 AM, "Ladislav Lhotka via Datatracker" <[email-masked]> wrote:

    [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdatatracker.ietf.org%2Fdoc%2Freview-ietf-spring-sr-yang-09-yangdoctors-early-lhotka-2018-10-24%2F&amp;data=02%7C01%7Cyingzhen.qu%40futurewei.com%7Cb6171db579ff4df2c02e08d8482c6cde%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637338702924404727&amp;sdata=%2BCVcN8RQQ%2B2aDP%2BFHY2GiCukPEzpf1GcJAtQ0LDbBh4%3D&amp;reserved=0

     Comments
    ------------

    - The title of Section 6 (States) still looks weird to me. My suggestion is to
    use "State Data" instead.
[YQ]: modified as you suggested.

    - The title of Section 8 should use plural "YANG Modules" because it contains
    three modules. It would also be helpful to introduce a subsection for each
    module.
[YQ]: changed the title and added an introduction of each module.

      <routing

[YQ]: I tried the format as your suggested, but somehow I could get it pass yanglint, so added "\" per RFC 8792.

[YQ]: Added JSON format.

---

From: Yingzhen Qu <[email-masked]>
Date: Wed, 26 Aug 2020 20:24:45 +0000

Hi Lada,

Thanks for the review.  I've uploaded version -22 to address your comments. Please see my answers inline for details.

Thanks,
Yingzhen

On 8/26/20, 1:59 AM, "Ladislav Lhotka" <[email-masked]> wrote:

    Hi Yingzhen,

    please see my responses inline.

    >     Review result: Ready with Nits

    ...

    > [YQ]: changed the title and added an introduction of each module.

    I meant to add a subsection of sec. 8 for each module so that they are clearly separated and each of them easily accessible from the table of contents.

[YQ]: sorry for the misunderstanding. I've added a subsection for each module, and it does look better now.
    > [YQ]: I tried the format as your suggested, but somehow I could get it pass yanglint, so added "\" per RFC 8792.

    OK.

    >     to provide it in the JSON representation per RFC 7951.

    Looks good, maybe only the first brace can be moved to the left.
[YQ]: fixed.

    Thanks, Lada

    -- 
    Ladislav Lhotka 
    Head, CZ.NIC Labs
    PGP Key ID: 0xB8F92B08A9F76C67