Skip to content
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

Add PathAndStruct helper for generated non-leaf PathStruct types. #533

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wenovus
Copy link
Collaborator

@wenovus wenovus commented May 26, 2021

A common use case is where a user would like to construct a path and populate its corresponding GoStruct at the same time. This helper obviates the need to retrieve the struct using the GetOrCreateXXX functions on top of using the path API.

This can be convenient for long paths such as the following example:

Before

d := &structAPI.Device{}
addr := d.GetOrCreateInterface("Ethernet0").GetOrCreateSubinterface(0).GetOrCreateIpv4().GetOrCreateAddress("10.10.10.10")
addr.PrefixLength = ygot.Uint8(24)
p, addr := pathAPI.Interface("Ethernet0").Subinterface(0).Ipv4().Address("10.10.10.10").Replace(addr)

After

p, addr := pathAPI.Interface("Ethernet0").Subinterface(0).Ipv4().Address("10.10.10.10").PathAndStruct()
addr.PrefixLength = ygot.Uint8(24)
p.Replace(addr)

This helper is not generated for leaves, since their types can be accessed using ygot helper functions such as ygot.Int32().

A common use case is where a user would like to construct a path and populate its corresponding `GoStruct` at the same time. This helper obviates the need to retrieve the struct using the `GetOrCreateXXX` functions on top of using the path API.
@googlebot googlebot added the cla: yes This PR author has signed the CLA label May 26, 2021
@wenovus wenovus marked this pull request as draft May 26, 2021 19:20
@wenovus wenovus marked this pull request as ready for review May 26, 2021 19:53
@coveralls
Copy link

coveralls commented May 26, 2021

Coverage Status

Coverage decreased (-0.01%) to 90.575% when pulling 319f976 on path-and-struct into 75ebd15 on master.

if err := goPathStructTemplate.Execute(&structBuf, structData); err != nil {
return GoPathStructCodeSnippet{}, util.AppendErr(errs, err)
}
if err := goPathAndStructHelperTemplate.Execute(&structBuf, structData); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should generating the path and struct helper be optional? In general we've tried to have these other features that are new types of output flag-controlled in the library - even if we have it as on by default, it'd probably be nice to have something whereby a user can turn this off if they don't want it to save on generated code size.

@@ -2040,6 +2075,11 @@ type List struct {
*ygot.NodePath
}

// PathAndStruct returns the path struct and an empty oc.List for the path "/root-module/list-container/list".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting -- for YANG container types it seems pretty clear to just return the struct, but in the case I call this with pathlib.ListContainer().ListAny().PathAndStruct() should I get back oc.List{} or map[...]*oc.List{}?

@greg-dennis
Copy link
Contributor

One alternative is for Replace on an intermediate node to always construct a new value and accept a lambda that can modify it, e.g.

pathAPI.Interface("Ethernet0").Subinterface(0).Ipv4().Address("10.10.10.10").Replace(t, func(addr A) {
  addr.PrefixLength = ygot.Uint8(24)
})

Not advocating for it, just thinking through the options.

@robshakir
Copy link
Contributor

Wen, is this change defunct at this point -- or should we try and merge it?

Copy link
Contributor

@greg-dennis greg-dennis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe we should include this in the space of changes that will be considered for a merged/revised gNMI API in H1. At this point, this feels a little too much of a piecemeal contribution to me, and we might benefit from incorporating it into the more holistic look that is planned.

@wenovus
Copy link
Collaborator Author

wenovus commented Nov 16, 2021

When generics is ready a better design for this might be just to provide the function

func ygot.GoStruct[T GoStruct](PathStruct[T]) T {}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This PR author has signed the CLA more-design-needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants