Last Call Review of draft-ietf-ccamp-optical-impairment-topology-yang-12
review-ietf-ccamp-optical-impairment-topology-yang-12-yangdoctors-lc-vasko-2023-04-04-00

The reviewed YANG module should describe the parameters it intends to describe
but there are lots of formal shortcomings in YANG formatting and general
consistency. To fix formatting, pyang tool can be used. Description texts
should be full sentences. Data examples are not valid (even include a note that
they need to be updated), use yanglint to validate them against the YANG
module. Other than that, some specific improvements:

## line 170: leaf out-voa
- units dB seems redundant, it is part of the used type name

## 326 - 379: grouping roadm-add-path
## 426 - 478: grouping roadm-drop-path
- all the 5 leaves seems to match exactly those in the grouping
roadm-express-path above so I would just put uses in this grouping

## 405, 571: leaf roadm-noise-figure
- lots of the leaves are using types from l0-types
- a local typedef can be defined (if no existing one can be used) that would
prevent duplication of all the information and move type specifics out of the
data nodes - alternatively a grouping with this leaf can be defined to avoid
description duplication but that is up to you, seems unnecessary

## 631: leaf nominal-power-spectral-density
- similar problem as the previous one except a grouping makes no sense, just a
typedef

## 287, 298, 635, 686:
- union with an empty type is used a lot and selecting this type is usually
explained but not in these cases

## 880: augment "/nw:networks/nw:network/nw:network-types/tet:te-topology"
- adding an empty presence container, which does not make sense on its own
unless other foreign augments are expected - following 2 augments add into this
container, why not merge all 3 augments into one to prevent any confusion?

## 899: container otsi-information
- including "information" in the identifier seems redundant

## 1293, 1315, 1335, 1359, 1412, 1468: leaf roadm-path-impairments
- all the leaves are "config false" when augments are applied but some have it
specified explicitly some not - if the leafref path is changed to an absolute
path, the leaf can be in a grouping and used at all the places

## 1492 - 1509; 1524 - 1541; 1570 - 1579; 1614 - 1623:
- both leaves add-path-impairments and drop-path-impairments use the same
absolute path leafref so a single typedef can be defined (also used for the
leafref in the leaf roadm-path-impairments above) - then both leaves can be put
into a grouping and used at all 4 places

Regards,
Michal