-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
issue-90: Fix 'augment' with module namespace issue. #92
Conversation
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 ***@***.***> 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
<#90> and #91
<#91>. With this change, both
of these two issues can be resolved.
However, as it actually reverts some of your change in revision 75b0325
<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.
------------------------------
You can view, comment on, or merge this pull request online at:
#92
Commit Summary
- issue-90: Fix 'augment' with module namespace issue.
File Changes
- *M* src/lang/extensions.coffee
<https://github.com/corenova/yang-js/pull/92/files#diff-0> (11)
- *M* test/extension/module.coffee
<https://github.com/corenova/yang-js/pull/92/files#diff-1> (2)
Patch Links:
- https://github.com/corenova/yang-js/pull/92.patch
- https://github.com/corenova/yang-js/pull/92.diff
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#92>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHIU3GJ4IHVWJFVBK3HP3DPUDG55ANCNFSM4HLEOP5Q>
.
|
Hi @saintkepha , I guess the intention of your changes would be trying to support the attributes with the full namespace, right? Such as:
As my change will make it only work on this way:
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:
I will take a look how the eval() function works on different naming. |
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 ***@***.***> 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
<#92 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAHIU3GYBWV35FGHDC2EX6LPUIDCRANCNFSM4HLEOP5Q>
.
|
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. Cheers. |
If the contextual property name can match, just use the contextual one, which should be more efficient than the fully qualified name. As the fully qualified name need to find out the module name from the origin or parent nodes.
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. |
That makes sense. I agree that adding the module name into the tag attribute is not so good. Cheers. |
…e cloned nodes to the augmenting module. property enforces fully-qualified name if it's externally augmented node, otherwise allows property to be set using fully-qualified name (module:tag) or just (tag)
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.