CESNET / libyang-cpp

C++ bindings for the libyang library
https://gerrit.cesnet.cz/q/project:CzechLight/libyang-cpp
BSD 3-Clause "New" or "Revised" License
10 stars 7 forks source link

Feature request: Wrapper for lyd_compare_single and lyd_compare_siblings #19

Closed rbu9fe closed 5 months ago

rbu9fe commented 10 months ago

As the title says the wrapper lacks support for lyd_compare_single and lyd_compare_siblings as the data node's operator== only compares whether the two nodes point to the very same item rather than comparing whether they have the same contents. Is there a chance to get this functionality?

jktjkt commented 10 months ago

Sure. This is an opportunity for getting the API "right", so I wonder which of these you would prefer. Given this definition:

auto node1 = ...;
auto node2 = ...;

Either a symbolic approach:

if (node1.siblings() == node2.siblings()) {
    // ...
}

if (node1.wholeTree() == node2.wholeTree()) {
    // ...
}

or a C-like approach:

if (siblingsEqual(node1, node2)) {
    // ...
}

if (subtreesEqual(node1, node2))  {
  // ...
}

In the symbolic approach, wholeTree would be just as proxy class for comparisons, while siblings is a Collection<T, U> already, so it's just a matter for adding a suitable comparison operator.

rbu9fe commented 10 months ago

Great! I guess I would prefer the symbolic approach, but I'm fine with whatever fits bests into the current design.

syyyr commented 10 months ago

Could also go with the "java" approach:

if (node1.siblingsEqual(node2)) {
    // ...
}

if (node1.subtreesEqual(node2)) {
    // ...
}

It's easier to find methods like these through code completion (more than the operator== or the free function version IMHO).

It would also sort of follow the principle of how we wrap the functions - foo(node, args...) turns into node.foo(args...).

Also, lyd_compare_single has a "Data compare options" argument. Would it be possible to specify this argument, if we were to use operator== version?

syyyr commented 10 months ago

Maybe I have missed that .wholeTree() is probably specifying the LYD_COMPARE_FULL_RECURSION flag... that would solve the options issue (but idk about LYD_COMPARE_DEFAULTS or LYD_COMPARE_OPAQ). Anyway, the Java approach is still an option.

rbu9fe commented 8 months ago

Just curious, is there any progress on this?

jktjkt commented 8 months ago

How does https://gerrit.cesnet.cz/c/CzechLight/libyang-cpp/+/6829 look like?

rbu9fe commented 8 months ago

Looks nice to me!