Ugh, this didn’t get formatted well at all. Resending here:
- YANG tree has subtle differences from the YANG module; regenerate
- Run the module through `pyang -f yang`
- No linting errors, though the copyright needs to be updated
- Pet peeve: I prefer descriptions to begin with a capital letter and end with a full-stop '.'
- Many descriptions (especially of identities) have an RFC in them. Please make this a reference as well.
- For fl-type, if this is a positive integer, why not use an unsigned type?
- appiid and deviid should be AppIID and DevIID as stated in RFC8724
- In grouping tv-struct's description: s/enconding/encoding/
- Leaf "index" in tv-struct grouping: s/indicia/index/
- In leaf field-position: s/occurence/occurrence/
- You use "YANG referenceid" a few times, but this isn't a thing per se. You tend to use
this to mean an identity reference. In all cases, though, I think it would be
best to more clearly state what the leaf is/does and leave out that fact that it
uses an identityref
- In grouping compression-rule-entry: s/identifer/identifier/, but again, I'd leave
out these YANG bits from descriptions.
- In your tv-struct grouping, define "index" as the first leaf. That seems a bit more logical to me.
- For leaf comp-decomp-action, your description is a tautology (which is another pet peeve of mine).
Can you sweep descriptions and make sure they provide some additional context or at least a reference?
- For something like di-type, does it make sense to be an identity? Seems like this could be an enumeration
as I don't think you'll have directions other than up, down, bi? But maybe you think
these types may be extended? Just curious.
- In leaf direction: s/bidirectionnal/bidirectional/ and s/forbiden/forbidden/
- For w-size, why not use derived-from-or-self here (same in other places where fragmentation-mode is referenced)?
- In grouping compression-content: s/identifed/identified/
- See Section 3.4 of https://datatracker.ietf.org/doc/html/rfc8407 on how to reference YANG tree diagrams
- Why do you have empty cases for the mode choice (e.g., no-ack, ack-always)? It's not clear you need a choice here.
And if you need it, you could just have one "when" clause on the case itself.
- There is another empty case for the no-compression case in the nature choice.
The description there says that a rule is required. - Can one have both
features for compression and fragmentation? The choice seems to imply no, but
I am curious. I didn't get the impression that they were mutually exclusive
from 8724.
From: yang-doctors <yang-doctors-bounces@xxxxxxxx> on behalf of Joe Clarke via Datatracker <noreply@xxxxxxxx>
Date: Friday, July 15, 2022 at 16:29
To: yang-doctors@xxxxxxxx <yang-doctors@xxxxxxxx>
Cc: draft-ietf-lpwan-schc-yang-data-model.all@xxxxxxxx <draft-ietf-lpwan-schc-yang-data-model.all@xxxxxxxx>, last-call@xxxxxxxx <last-call@xxxxxxxx>, lp-wan@xxxxxxxx <lp-wan@xxxxxxxx>
Subject: [yang-doctors] Yangdoctors last call review of draft-ietf-lpwan-schc-yang-data-model-14
Reviewer: Joe Clarke
Review result: Not Ready
I have been tasked to perform a LC review on this draft on behalf of YANG
Doctors. This document defines a YANG module to codify the rules for Static
Context Header Compression. While I say that it is "Not Ready" from a LC
perspective, it may be more ready than that lets on. I have a few questions of
the authors to gain perspective on what they're trying to achieve. I have also
found a number of issues in the module itself. See below:
- YANG tree has subtle differences from the YANG module; regenerate
- Run the module through `pyang -f yang` to generate a canonical formatted
version - No linting errors, though the copyright needs to be updated - Pet
peeve: I prefer descriptions to begin with a capital letter and end with a
full-stop '.' - Many descriptions (especially of identities) have an RFC in
them. Please make this a reference as well. - For fl-type, if this is a
positive integer, why not use an unsigned type? - appiid and deviid should be
AppIID and DevIID as stated in RFC8724 - In grouping tv-struct's description:
s/enconding/encoding/ - Leaf "index" in tv-struct grouping: s/indicia/index/ -
In leaf field-position: s/occurence/occurrence/ - You use "YANG referenceid" a
few times, but this isn't a thing per se. You tend to use
this to mean an identity reference. In all cases, though, I think it would be
best to more clearly state what the leaf is/does and leave out that fact that
it uses an identityref
- In grouping compression-rule-entry: s/identifer/identifier/, but again, I'd
leave
out these YANG bits from descriptions.
- In your tv-struct grouping, define "index" as the first leaf. That seems a
bit more logical to me. - For leaf comp-decomp-action, your description is a
tautology (which is another pet peeve of mine).
Can you sweep descriptions and make sure they provide some additional context
or at least a reference?
- For something like di-type, does it make sense to be an identity? Seems like
this could be an enumeration
as I don't think you'll have directions other than up, down, bi? But maybe
you think these types may be extended? Just curious.
- In leaf direction: s/bidirectionnal/bidirectional/ and s/forbiden/forbidden/
- For w-size, why not use derived-from-or-self here (same in other places where
fragmentation-mode is referenced)? - In grouping compression-content:
s/identifed/identified/ - See Section 3.4 of
https://datatracker.ietf.org/doc/html/rfc8407 on how to reference YANG tree
diagrams - Why do you have empty cases for the mode choice (e.g., no-ack,
ack-always)? It's not clear you need a choice here.
And if you need it, you could just have one "when" clause on the case itself.
- There is another empty case for the no-compression case in the nature choice.
The description there says that a rule is required. - Can one have both
features for compression and fragmentation? The choice seems to imply no, but
I am curious. I didn't get the impression that they were mutually exclusive
from 8724.
_______________________________________________
yang-doctors mailing list
yang-doctors@xxxxxxxx
https://www.ietf.org/mailman/listinfo/yang-doctors
