Re: [Teas] Yangdoctors early review of draft-ietf-teas-yang-path-computation-11

["Belotti, Sergio (Nokia - IT/Vimercate)" <[e-mail]>]

Hi Martin,
Thanks for your review, we have addressed them in the version-12 we have just submitted.
See our feedbacks and some follow up questions in line marked with "Authors>".

Thanks
Italo and Sergio

> -----Original Message-----
> From: Martin Björklund via Datatracker <[e-mail]>
> Sent: Thursday, December 17, 2020 7:59 PM
> To: [e-mail]
> Cc: [e-mail]; [e-mail]
> Subject: Yangdoctors early review of draft-ietf-teas-yang-path-computation-
> 11
> 
> Reviewer: Martin Björklund
> Review result: Ready with Nits
> 
> 
> o  General
> 
>   The language is called "YANG", not "Yang".

Authors> ok

> 
> 
> o  1.2. Tree Diagram
> 
>   The text says:
> 
>     A simplified graphical representation of the data model is used in
>     section 6.1 of this this document.  The meaning of the symbols in
>     these diagrams is defined in [RFC8340].
> 
>   Tree diagrams are used also in chapter 5.  I suggest:
> 
>     Tree diagrams used in this document follow the notation defined in
>     [RFC8340].

Authors> ok

> 
> 
> o  Tree diagrams in general
> 
>    You can use pyang -f tree --tree-line-length 68 ... in order to
>    avoid long lines in the RFC.

Authors> we are using the same pyang string but with 69 limit, but it seems not always (or for all lines) working. Why 68 and not 69? Why something should change?

> 
> 
> o  6.1
> 
>   This section presents a fully expanded tree diagram of the module.
>   Tree diagrams are mainly used to give an overview of a module's
>   structure.  The tree diagram in this section spans 13 pages and is
>   quite hard to read.
> 
>   I also note that a majority of the nodes in this tree diagram come
>   from the expansion of groupings that aren't defined in this
>   document.  Hence, I suggest that you might want to run:
> 
>     pyang -f tree --tree-line-length 68 \
>       --tree-print-groupings --tree-no-expand-uses
> 

Authors> we have tried but it seems that also the groupings defined in the module are not expanded and we think at least these groupings should be expanded in the tree.

> 
> o  There are a number of groupings that are used only once, and do not
>    seem to be defined to be reused by other modules, e.g.,
>    "requested-info", "requested-state", "svec-metrics-bounds" and more.
> 
>    If they are intended to be reused, it should be made clear in their
>    description statements.  If not, I think they should be inlined and
>    removed.

Authors> We think that it is sometimes necessary to introduce groupings even if they are not re-used to make the YANG code structured and readable. This is the same logic in other programming, where "helper" functions are used to code complex logics.

Authors> Following this approach we have kept the following groupings:

- requested-info
- requested-state (moving the container statement inside)
- reported-state
- synchronization-constraints
- synchronization-optimization
- synchronization-info

and removed the following groupings:

- svec-metrics-bounds
- svec-metrics-optimization
- svec-exclude

> 
> 
> o  grouping svec-exclude
> 
>    This grouping has an ordered-by user list.  Why is this list user
>    ordered?  If the order matters, it should be explained how it matters.

Authors> you're right. Deleted "ordered-by user list". No reason for that.

> 
>    Also, the index leaf has this description:
> 
>      "XRO subobject index"
> 
>    What is "XRO"?  Is this description sufficiently clear?

Authors> We have removed the index since lists in RPC input do not need it.

> 
> 
> o  path-request
> 
>    In the path-request, there is construct for path-refs:
> 
>                    list primary-reverse-path-ref {
>                      key index;
>                      min-elements 1;
>                      description
>                        "The list of primary reverse paths that
>                         reference this path as a candidate
>                         secondary reverse path";
>                      leaf index {
>                        type uint32;
>                        description
>                          "The index used by the
>                           primary-reverse-path-ref list";
>                      }
> 
> 
>   What is this index?  Is it only used as an arbitrary index, or
>   something else?  If it is an arbitraty index, it should be explained
>   in the descriptions.
> 
>   Also note that lists in rpc input don't need an index.

Authors> OK, deleted. We have removed index both in "primary reverse path ref" and "primary path ref".

> 
> 
> o  Validation
> 
>    The module fails YANG validation, but that is really due to errors
>    in [e-mail].  Specifially, the leafref in the
>    grouping "path-compute-info" must have prefixes in its path.
>    Without prefixes, the path refers to nodes in the module that uses
>    the grouping.  (same for other groupings in that module).

Authors> See https://github.com/tsaad-dev/te/issues/114


> 
> 
> o  Layout
> 
>   I suggest you run the module through
> 
>     pyang -f yang --yang-canonical --yang-line-length 68
> 
>   in order to have the module indented and formatted consistently.
> 
>   This will make the RFC editor's job easier.

Authors> We are using " pyang --strict --ietf --max-line-length 69 -f tree --tree-line-length=69 -o ietf-te-path-computation.tree ietf-te-path-computation.yang". Is something wrong with this string command?

> 
> 
> 
> /martin
> 
>