Re: [yang-doctors] Yangdoctors last call review of draft-ietf-netconf-rfc7895bis-04

["Reshad Rahman (rrahman)" <[e-mail]>]

Hi,

I agree with your responses (I realize the self-referential question is being discussed with Martin). 

I think it'd be good to have a common type for date so that it doesn't have to be re-defined in multiple places. But this is orthogonal to the discussion on this document.

Regards,
Reshad.

On 2018-02-21, 11:18 AM, "Juergen Schoenwaelder" <[e-mail]> wrote:

    Reshad,
    
    thanks for your review. Comments inline...
    
    On Mon, Feb 19, 2018 at 03:30:48PM -0800, Reshad Rahman wrote:
    > Reviewer: Reshad Rahman
    > Review result: Ready with Nits
    > 
    > YANG Doctor review of draft-ietf-netconf-rfc7895bis-04 by Reshad Rahman.
    > 
    > 1 module in this draft:
    > - [e-mail]
    > 
    > YANG validation on 2018-02-18 reports 2 errors but no warnings. This comes from
    > yanglint 0.14.69 and seems to be a tool issue: err : Module "ietf-yang-library"
    > in another revision already implemented. err : Module "ietf-yang-library"
    > parsing failed.
    
    Yes, I did run into this as well when I tried to use yanglint to
    validate the examples. Its a tool issue which is specific to this
    module (hence perhaps not worth fixing).
    
    > 2 examples are provided in this draft (Appendices B and C).
    > 
    > Module [e-mail]:
    > - typedef revision-identifier. The definition allows invalid dates such as
    > 2018-99-99, has there been any consideration to use a more restrictive
    > definition, e.g. ‘\d{4}-[0-1][0-9]-[0-3][0-9]’ (I realize this still allows
    > incorrect dates)?  Also the revision timestamp is just a date which follows
    > YYYY-MM-DD format, isn’t there an existing common typedef (as there is for
    > date-and-time) which can be reused?
    
    It is always somewhat subjective how restrictive you make a pattern.
    Note that the pattern for the date-and-time typedef in RFC 6991 also
    uses '\d{4}-\d{2}-\d{2}' so I think we are consistent. Yes, there is
    no common 'date' type yet - hence we define things inline.
    
    > - grouping implementation-parameters should be renamed
    > module-implementation-parameters?
    
    This name change may make sense.
    
    > - The groupings defined are used only once. For
    > yang-library-parameters I see why this is done since it can be
    > reused. But for others such as implementation-parameters, I don’t
    > see why a grouping is needed. Maybe this is just authors’ preference
    > to regroup information.
    
    I am not sure which modules would be using these groupings and whether
    it is necessary to provide these groupings. I know that some versions
    of schema mount used some of the groupings but this may not be
    necessary anymore with the new library structure. I assume Martin
    knows more details about which groupings may be useful to provide for
    possible reuse reasons.
    
    > - Description of leaf module in list deviation: by
    > “self-referential” I assume that this means that the reference can
    > not refer to the ietf-yang-library module? While this may seem
    > obvious I believe it’d be good to spell it out.
    
    No, the intention was to say that a module should not list itself as a
    deviation module, neither directly or indirectly. I guess we have to
    describe this better. Perhaps something along these lines:
    
            description
              "A module that deviates the module associated with this
               entry. This reference MUST NOT (directly or indirectly)
               refer to the module being deviated.
    
               Clients, MUST make sure that they handle a situation
               where a module deviates itself (directly or indirectly)
               gracefully.";
    
    > - list module-set. Since this is for schemas, consider renaming it
    > to something along the lines of schema-module-set?
    
    While modules sets are used for schemas, I do not see why a list name
    should be selected to constrain this. Actually, changing the name does
    not do anything other than changing the name. ;-)
    
    > - Use of “non-import module” in a few descriptions. Should this be
    > “non import-only modules”?
    
    Yes, I have changed the sources.
    
    > Nits:
    > - Section 1, 2nd paragraph s/informaton/information/
    
    fixed
    
    > - Section 1, 3rd paragraph, add “” quote around /module-states.
    
    fixed
    
    > - Section 1, 5th paragraph s/Since the YANG library contents
    > changes/Since the YANG library contents change/?
    
    I would leave this to the RFC editor; is 'content' countable here or
    not, should it be plural or singular? 
    
    > - Section 4, 2nd paragraph “…that individual datastore schema
    > share..”. Should that be “… that individual datastore schemas
    > share..”?
    
    I added a plural 's' but again I am happy to let the RFC editor decide
    plural form here.
    
    - Appendix B and C. The examples use YANG models which are still
    > being updated (hence the note to RFC Editor). Why not use a YANG model which is
    > stable, is this because of need to have an NMDA compliant module in the
    > examples?
    
    These models are NMDA versions and they are in the RFC editor queue so
    I think this is (a) a reason for using them and (b) the references
    will be RFCs by the time this document appears.
    
    /js
    
    -- 
    Juergen Schoenwaelder           Jacobs University Bremen gGmbH
    Phone: [phone]         Campus Ring 1 | 28759 Bremen | Germany
    Fax:   [phone]         <https://www.jacobs-university.de/>