search

corenova / yang-js

YANG parser and composer
Apache License 2.0
56 stars 18 forks source link

issue-90: Fix 'augment' with module namespace issue. #92

Closed quantang closed 5 years ago

quantang commented 5 years ago

Hi @saintkepha ,

After I went through you changelogs, I found this change should be related to the namespace issue that I mentioned in #90 and #91. With this change, both of these two issues can be resolved. However, as it actually reverts some of your change in revision 75b0325, I am not sure whether it is some of our different understanding of the YANG schema data.

Please take a look and share your thoughts. Cheers.

sekur commented 5 years ago

Thanks @quantang I'll review tomorrow and let you know. There's a reason I've made the change and will need to reconcile with the reported issues.

Best, Peter

On Mon, May 6, 2019 at 5:50 PM Quan Tang notifications@github.com wrote:

Hi @saintkepha https://github.com/saintkepha ,

After I went through you changelogs, I found this change should be related to the namespace issue that I mentioned in #90 https://github.com/corenova/yang-js/issues/90 and #91 https://github.com/corenova/yang-js/issues/91. With this change, both of these two issues can be resolved. However, as it actually reverts some of your change in revision 75b0325 https://github.com/corenova/yang-js/commit/75b03253591c21c80314cbb30b8f940839b28031, I am not sure whether it is some of our different understanding of the YANG schema data.

Please take a look and share your thoughts. Cheers.

You can view, comment on, or merge this pull request online at:

https://github.com/corenova/yang-js/pull/92 Commit Summary

  • issue-90: Fix 'augment' with module namespace issue.

File Changes

Patch Links:

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/corenova/yang-js/pull/92, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHIU3GJ4IHVWJFVBK3HP3DPUDG55ANCNFSM4HLEOP5Q .

quantang commented 5 years ago

Hi @saintkepha ,

I guess the intention of your changes would be trying to support the attributes with the full namespace, right? Such as:

"A:group": {
  "B:group" : {
    "B:leaf": "value"
  }
}

As my change will make it only work on this way:

"A:group: {
  "B:group" : {
    "leaf": "value"
  }
}

I will take a look at whether we can support both of them.

I tried the unit test on "module.coffee", it shows the locate() function works well on both scenario, as follows:

  it "should parse augment module statement", ->
    y1 = Yang.use (Yang.parse schema1)
    y2 = Yang.parse schema2
    y2.locate('/foo:c1/foo:c2/bar:c3/bar:a2').should.have.property('tag').and.equal('a2')  // passed
    y2.locate('/foo:c1/c2/bar:c3/a2').should.have.property('tag').and.equal('a2')  // passed

I will take a look how the eval() function works on different naming. Cheers.

sekur commented 5 years ago

Yes, the intended change was to decouple tag from namespace so that we can be more flexible on how we treat fully qualified property names vs contextual property names.

We may need to have a different handler to honor the augment case where the cloned schema nodes should be tied to the originating schema module vs the target schema module.

Thanks for looking into a potential solution!

Peter

On Tue, May 7, 2019 at 4:03 PM Quan Tang notifications@github.com wrote:

Hi @saintkepha https://github.com/saintkepha ,

I guess the intention of your changes would be trying to support the attributes with the full namespace, right? Such as:

"A:group": { "B:group" : { "B:leaf": "value" } }

As my change will make it only work on this way:

"A:group: { "B:group" : { "leaf": "value" } }

I will take a look at whether we can support both of them.

I tried the unit test on "module.coffee", it shows the locate() function works well on both scenario, as follows:

it "should parse augment module statement", -> y1 = Yang.use (Yang.parse schema1) y2 = Yang.parse schema2 y2.locate('/foo:c1/foo:c2/bar:c3/bar:a2').should.have.property('tag').and.equal('a2') // passed y2.locate('/foo:c1/c2/bar:c3/a2').should.have.property('tag').and.equal('a2') // passed

I will take a look how the eval() function works on different naming. Cheers.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/corenova/yang-js/pull/92#issuecomment-490285389, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHIU3GYBWV35FGHDC2EX6LPUIDCRANCNFSM4HLEOP5Q .

quantang commented 5 years ago

Hi @saintkepha ,

I added another change, which should make the yang-js to support both fully qualified property names and the contextual property names. It also includes the unit test on this scenario. Please take a look and correct me if there is anything wrong.

Cheers.

sekur commented 5 years ago

@quantang

Can we try to handle without reverting the change to augment statement transform inside extensions.coffee? I want to avoid having the module name prepended into .tag at the schema level. The reason is that when we have multiple uses/augment nested across multiple schema modules we end up with invalid prepended tag, (i.e. some-module:another-module:foo).

I think the changes to Properly class to check originating object for FQN or contextual name during .attach for initial .set should be the only necessary fix.

quantang commented 5 years ago

That makes sense. I agree that adding the module name into the tag attribute is not so good. I will take a look at how it may behaviour if I change it back.

Cheers.