Skip to content

Commit

Permalink
Enhancing Choice/Case options
Browse files Browse the repository at this point in the history 
This patch allows user to use with_choices option in children() calls
  • Loading branch information
@steweg
steweg committed Jan 26, 2024
1 parent 3e3af68 commit e44fca8
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 12 deletions.
12 changes: 12 additions & 0 deletions cffi/cdefs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1006,6 +1006,18 @@ struct lysc_when {

struct lysc_when** lysc_node_when(const struct lysc_node *);

struct lysc_node_case {
struct lysc_node *child;
struct lysc_when **when;
...;
};

struct lysc_node_choice {
struct lysc_node_case *cases;
struct lysc_when **when;
...;
};

This comment has been minimized.

Sign in to view

Copy link
@samuel-gauthier

samuel-gauthier Jan 27, 2024

Collaborator

I don't think this is needed (see below).
#define LYD_DEFAULT ...
#define LYD_WHEN_TRUE ...
#define LYD_NEW ...
Expand Down
52 changes: 40 additions & 12 deletions libyang/schema.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,11 @@ def get_module_from_prefix(self, prefix: str) -> Optional["Module"]:
def __iter__(self) -> Iterator["SNode"]:
return self.children()

def children(self, types: Optional[Tuple[int, ...]] = None) -> Iterator["SNode"]:
return iter_children(self.context, self.cdata, types=types)
def children(
self, types: Optional[Tuple[int, ...]] = None, with_choices: bool = False

This comment has been minimized.

Sign in to view

Copy link
@samuel-gauthier

samuel-gauthier Jan 27, 2024

Collaborator

That is a lot of code modified, that will need to be modified each time we add an option. I wonder if it wouldn't be nicer to give the options directly, and to define the existing options in the SNode class. @rjarry?

This comment has been minimized.

Sign in to view

Copy link
@steweg

steweg Jan 29, 2024

Author Contributor

I know it is not my decision, but I agree with your proposal of exposing the options in all of those functions + adding general function which converts explicit options to int value (so caller doesn't need to access lib.LY* staff

But this is not inline with how similar other options/flags are being done (e.g. data.py/merge_flags), there all the external API calls always includes ability to add options directly within the API on option-by-option basis. So that's why I have done this patch in this way as it is...
) -> Iterator["SNode"]:
options = lib.LYS_GETNEXT_WITHCHOICE if with_choices else 0
return iter_children(self.context, self.cdata, types=types, options=options)

def __str__(self) -> str:
return self.name()
Expand Down Expand Up @@ -1357,18 +1360,30 @@ def must_conditions(self) -> Iterator[str]:
def __iter__(self) -> Iterator[SNode]:
return self.children()

def children(self, types: Optional[Tuple[int, ...]] = None) -> Iterator[SNode]:
return iter_children(self.context, self.cdata, types=types)
def children(
self, types: Optional[Tuple[int, ...]] = None, with_choices: bool = False
) -> Iterator[SNode]:
options = lib.LYS_GETNEXT_WITHCHOICE if with_choices else 0
return iter_children(self.context, self.cdata, types=types, options=options)


# -------------------------------------------------------------------------------------
@SNode.register(SNode.CHOICE)
class SChoice(SNode):
__slots__ = ("cdata_choice",)

def __init__(self, context: "libyang.Context", cdata):
super().__init__(context, cdata)
self.cdata_choice = ffi.cast("struct lysc_node_choice *", cdata)

This comment has been minimized.

Sign in to view

Copy link
@samuel-gauthier

samuel-gauthier Jan 27, 2024

Collaborator

You are not using this, can you remove it?

This comment has been minimized.

Sign in to view

Copy link
@steweg

steweg Jan 29, 2024

Author Contributor

Done
def __iter__(self) -> Iterator[SNode]:
return self.children()

def children(self, types: Optional[Tuple[int, ...]] = None) -> Iterator[SNode]:
return iter_children(self.context, self.cdata, types=types)
def children(
self, types: Optional[Tuple[int, ...]] = None, with_cases: bool = False
) -> Iterator[SNode]:
options = lib.LYS_GETNEXT_WITHCASE if with_cases else 0
return iter_children(self.context, self.cdata, types=types, options=options)


# -------------------------------------------------------------------------------------
Expand All @@ -1377,8 +1392,11 @@ class SCase(SNode):
def __iter__(self) -> Iterator[SNode]:
return self.children()

def children(self, types: Optional[Tuple[int, ...]] = None) -> Iterator[SNode]:
return iter_children(self.context, self.cdata, types=types)
def children(
self, types: Optional[Tuple[int, ...]] = None, with_choices: bool = False
) -> Iterator[SNode]:
options = lib.LYS_GETNEXT_WITHCHOICE if with_choices else 0
return iter_children(self.context, self.cdata, types=types, options=options)


# -------------------------------------------------------------------------------------
Expand All @@ -1398,9 +1416,15 @@ def __iter__(self) -> Iterator[SNode]:
return self.children()

def children(
self, skip_keys: bool = False, types: Optional[Tuple[int, ...]] = None
self,
skip_keys: bool = False,
types: Optional[Tuple[int, ...]] = None,
with_choices: bool = False,
) -> Iterator[SNode]:
return iter_children(self.context, self.cdata, skip_keys=skip_keys, types=types)
options = lib.LYS_GETNEXT_WITHCHOICE if with_choices else 0
return iter_children(
self.context, self.cdata, skip_keys=skip_keys, types=types, options=options
)

def keys(self) -> Iterator[SNode]:
node = lib.lysc_node_child(self.cdata)
Expand Down Expand Up @@ -1498,15 +1522,19 @@ def iter_children(
options: int = 0,
) -> Iterator[SNode]:
if types is None:
types = (
types = [
lib.LYS_ACTION,
lib.LYS_CONTAINER,
lib.LYS_LIST,
lib.LYS_RPC,
lib.LYS_LEAF,
lib.LYS_LEAFLIST,
lib.LYS_NOTIF,
)
]
if options & lib.LYS_GETNEXT_WITHCHOICE:
types.append(lib.LYS_CHOICE)

This comment has been minimized.

Sign in to view

Copy link
@samuel-gauthier

samuel-gauthier Jan 27, 2024

Collaborator

Do you see any problem with putting lib.LYS_CHOICE and lib.LYS_CASE in the types variable, and keep it as a tuple? It does not induce a behavior change, as anyway you have to set lib.LYS_GETNEXT_WITHCHOICE and lib.LYS_GETNEXT_WITHCASE to see them, right?

This comment has been minimized.

Sign in to view

Copy link
@steweg

steweg Jan 29, 2024 •
edited
Loading

Author Contributor

Well let me give you use-case when I am using it. In my project, I am creating like a dropdown menu for each choice, so user must first indicate the value of case and only then he/she see the real fields associated with given case. To do that I need to read the schema including all the choices & cases. To do that you need to use lib.LYS_GETNEXTWITHCHOICE and lib.LYS_GETNEXTWITHCASE. This would be enough if I would be using the libyang API directly. But libyang-python introduced filtering based on types, with some implicit default types. So I need to either specify all the possible types and also the options. Which seems to somehow complicate it. So my ideal proposal would be to keep the implicit default set modification of types based on options. Or if not, then define this as a some variable within the scope, which can be later on referenced and modified by user like the following

class SNode():
    ....
    ITER_CHILDREN_DEFAULT = [
            lib.LYS_ACTION,
            lib.LYS_CONTAINER,
            lib.LYS_LIST,
            lib.LYS_RPC,
            lib.LYS_LEAF,
            lib.LYS_LEAFLIST,
            lib.LYS_NOTIF,
        ]
    ....

def main():
    ....
    my_types = SNode.ITER_CHILDREN_DEFAULT.copy().append(SNode.CHOICE)
    iter_children(ctx, node, types=my_types)
    ....

This comment has been minimized.

Sign in to view

Copy link
@samuel-gauthier

samuel-gauthier Jan 29, 2024

Collaborator

Or the default value for types could be all the types, which are available in SNode.NODETYPE_CLASS.keys() I believe. I am not sure why we filter by default, I don't think we should filter if the user does not specify types. Maybe @rjarry has some idea here?

This comment has been minimized.

Sign in to view

Copy link
@steweg

steweg Jan 29, 2024

Author Contributor

But you are right, adding those types into the private tuple doesn't make anything worse. So I have reworked as suggested
if options & lib.LYS_GETNEXT_WITHCASE:
types.append(lib.LYS_CASE)

def _skip(node) -> bool:
if node.nodetype not in types:
Expand Down
6 changes: 6 additions & 0 deletions tests/test_schema.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,12 @@ def test_cont_iter(self):
def test_cont_children_leafs(self):
leafs = list(self.container.children(types=(SNode.LEAF,)))
self.assertEqual(len(leafs), 9)
without_choices = [
c.name() for c in self.container.children(with_choices=False)
]
with_choices = [c.name() for c in self.container.children(with_choices=True)]
self.assertTrue("pill" not in without_choices)
self.assertTrue("pill" in with_choices)

def test_cont_parent(self):
self.assertIsNone(self.container.parent())
Expand Down

0 comments on commit e44fca8

Please sign in to comment.