mbj4668 / pyang

An extensible YANG validator and converter in python
ISC License
535 stars 344 forks source link

pyang silently drops populated "} //" entries appearing just prior to "} // augment" entries #760

Closed lbartholomew-rpc closed 2 years ago

lbartholomew-rpc commented 3 years ago

Hello. As discovered by Xufeng Li in RFC-to-be 9128: "The tool seems to silently drop one of the end-tags in each module: “// pim” in ietf-pim-base; “// rp” in ietf-pim-rp, etc."

Please see the following for examples (search for "} // augment"):

https://www.rfc-editor.org/authors/ietf-pim-base-rfcdiff.html https://www.rfc-editor.org/authors/ietf-pim-rp-rfcdiff.html https://www.rfc-editor.org/authors/ietf-pim-sm-rfcdiff.html https://www.rfc-editor.org/authors/ietf-pim-dm-rfcdiff.html https://www.rfc-editor.org/authors/ietf-pim-bidir-rfcdiff.html

wlupton commented 3 years ago

Try removing any comments before the module / submodule statement and after the last closing brace. Does that avoid the problem?

(Explanation: At least at one point, comments as described above were a problem because there was an assumption that there was only one top-level statement: a module / submodule statement. Such comments broke that assumption.)

mbj4668 commented 3 years ago

No, that's not the problem. I can reproduce the issue with this module:

module a {
  namespace "urn:a";
  prefix a;

  container x;

  augment "/x" {
    container dm {
      presence "x";
    } // dm
  } // augment
}

Changing the augment to augment "x" makes the problem go away.

Side note. I wonder how useful these "end" comments are. Have a look at these examples from the modules above:

    container dm {
      presence "Present to enable dense-mode.";
      description
        "PIM DM configuration data.";
    } // Dm
    container dm {
      presence "Present to enable dense-mode.";
      description
        "PIM DM configuration data.";
    } // sm

Clearly nobody pays any attention to these comments...

lbartholomew-rpc commented 3 years ago

Hi, Martin. Thanks for the note.

(As for "// Dm" and "// sm" in a dm container, these were fixed during editing.)

Thanks again to you and William for looking at this.

RFC Editor/lb

On Sep 21, 2021, at 11:47 AM, Martin Bjorklund @.***> wrote:

No, that's not the problem. I can reproduce the issue with this module:

module a { namespace "urn:a"; prefix a;

container x;

augment "/x" { container dm { presence "x"; } // dm } // augment }

Changing the augment to augment "x" makes the problem go away.

Side note. I wonder how useful these "end" comments are. Have a look at these examples from the modules above:

container dm {
  presence "Present to enable dense-mode.";
  description
    "PIM DM configuration data.";
} // Dm

container dm {
  presence "Present to enable dense-mode.";
  description
    "PIM DM configuration data.";
} // sm

Clearly nobody pays any attention to these comments...

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

On Sep 21, 2021, at 10:16 AM, William Lupton @.***> wrote:

Try removing any comments before the module / submodule statement and after the last closing brace. Does that avoid the problem?

(Explanation: At least at one point, comments as described above were a problem because there was an assumption that there was only one top-level statement: a module / submodule statement. Such comments broke that assumption.)

mbj4668 commented 3 years ago

@fredgan can you have a look at this?

fredgan commented 2 years ago

Well, I researched it for this. The "parent" property will be modified to the target node if the "augment" statement's argument is correct since the augment will expand. However, the comment printing need the "parent" property for level judging. if the "parent" is changed, it don't know they are in the same level.

@mbj4668 As I know, the "parent" property is used purely for statement tree. Should we change the parent property for augment expanding?

mbj4668 commented 2 years ago

I don't think we can change the 'parent' property; too much code relies on the current semantics. But would it be possible to tie the line_end '_comment' stmt to its "parent" statement in yang_parser.py, with a new attribute? Perhaps stmt.cmt_parent = parent or something similar.

fredgan commented 2 years ago

@lbartholomew-rpc sorry for long waiting. This bug has been just fixed in the latest master branch. Please check it.

lbartholomew-rpc commented 2 years ago

@fredgan -- I don't know GitHub. Where do I find the updated pyang so we can install it? Thank you.

fredgan commented 2 years ago

@fredgan -- I don't know GitHub. Where do I find the updated pyang so we can install it? Thank you.

Do you use version from PyPI ? If so, it may need a new version release.

lbartholomew-rpc commented 2 years ago

Hi, Fred. We have asked our programmer to install your new version of pyang from GitHub. Thank you.