add support for lyd_attr #77
Conversation
libyang/data.py
Outdated
| @@ -204,6 +204,7 @@ def __init__(self, context: "libyang.Context", cdata): | |||
| """ | |||
| self.context = context | |||
| self.cdata = cdata # C type: "struct lyd_node *" | |||
| self.cdata_attrs = [] # C tupe: "struct lyd_attr *" | |||
There was a problem hiding this comment.
Please fix the typo here ("tupe" => "type").
There was a problem hiding this comment.
Done in the DNodeAttrs class
tests/test_data.py
Outdated
| dnode = self.ctx.parse_data_mem(self.JSON_CONFIG, "json", validate_present=True) | ||
|
|
||
| hostname = dnode.find_one("hostname") | ||
| hostname.free(with_siblings=False) | ||
|
|
There was a problem hiding this comment.
Same question as the other commit, can you explain why you load the configuration, and delete the hostname in it at the beginning of your test? Why not start with an empty configuration instead?
There was a problem hiding this comment.
Done for all the tests
libyang/data.py
Outdated
| def attr(self): | ||
| ret = {} | ||
| for attr in self.cdata_attrs: | ||
| key = self._get_attr_key(attr) | ||
| ret[key] = c2str(attr.value) | ||
| return ret | ||
|
|
||
| def new_attr(self, name: str, value: str): | ||
| attrs = ffi.new("struct lyd_attr **") | ||
| ret = lib.lyd_new_attr( | ||
| self.cdata, | ||
| ffi.NULL, | ||
| str2c(name), | ||
| str2c(value), | ||
| attrs, | ||
| ) | ||
| if ret != lib.LY_SUCCESS: | ||
| raise self.context.error("cannot create attr") | ||
| self.cdata_attrs.append(attrs[0]) | ||
|
|
||
| def attr_free(self, name): | ||
| for attr in self.cdata_attrs: | ||
| key = self._get_attr_key(attr) | ||
|
|
||
| if name == key: | ||
| lib.lyd_free_attr_single(self.context.cdata, attr) | ||
| self.cdata_attrs.remove(attr) | ||
| break |
There was a problem hiding this comment.
I wonder if we need a dedicated class for those attributes. @rjarry, what do you think?
There was a problem hiding this comment.
That may be overkill. These attributes don't make sense without a host DNode.
There was a problem hiding this comment.
I sent my message too fast :D
Actually, yes. It would be better to have:
dnode.attrs().get("foo")
for key, val in dnode.attrs():
print(key, val)
dnode.attrs().set("foo", "bar")
dnode.attrs().remove("foo")API stub:
class DNodeAttrs:
def get(self, name: str) -> str:
...
def set(self, name: str, value: str):
...
def remove(self, name: str):
...
def __contains__(self, name: str):
...
def __iter__(self):
...
def __len__(self):
...
class DNode:
def attrs(self) -> DNodeAttrs:
...What do you think?
There was a problem hiding this comment.
Yes, I had something like that in mind. Agreed.
There was a problem hiding this comment.
I implemented your API suggestion
|
Could you rebase that on the current master when you submit the changes? |
In case of a call to DNode.new with cdata containing an opaque node the new method tries to access cdata.schema.nodetype, which results in a NULL pointer dereference. To get the schema for an opaque node retrieve the schema from the context using the path of the node. Fixes: CESNET#73 Signed-off-by: nvxf <[email protected]>
nvxf
force-pushed
the
feat/add-new-attr
branch
from
d3e1db2 to
d571671
Compare
|
Can you check what it fails on 3.6? |
The import |
|
I prefer moving the DNoteAttrs class. |
data: add support for lyd_attr to DNode Signed-off-by: nvxf <[email protected]>
nvxf
force-pushed
the
feat/add-new-attr
branch
from
d571671 to
0d74391
Compare
|
Done |
No description provided.