Refactor how path structs are split.

[robshakir]

* (M) ypathgen/pathgen.go
 * (M) ypathgen/pathgen_test.go
   - Change splits to include N structs per file based on iterating
     through them rather than precalculating.
   - Avoid creating files that are just the package header.
 * (A) ypathgen/testdata/splitstructs/*
   - Add new test case files.
   - Remove files that contained only the header.

[coveralls]

Coverage decreased (-0.002%) to 90.489% when pulling d33236c58a9e83e423de00372ddf138ea87d0d29 on splitn into f9326f6ef0cf10be5263b769a0253b607b8318b9 on master.

[wenovus]

FYI https://github.com/openconfig/ygnmi/tree/main/pathgen is the future place for ypathgen and pathgen.go here will eventually be deleted. I will upstream this change after merge.

[robshakir]

Good point about generating the empty files -- it's a bit messy from the perspective of non-bazel users to have a bunch of files sitting around, should we consider having a flag as to whether to write those?

w.r.t where this code should live, I can create the PR in the upstream if you'd prefer? At what point will there stop being a fork?

[wenovus]

Good point about generating the empty files -- it's a bit messy from the perspective of non-bazel users to have a bunch of files sitting around, should we consider having a flag as to whether to write those?

w.r.t where this code should live, I can create the PR in the upstream if you'd prefer? At what point will there stop being a fork?

Personally I think it's ok to just have them lying around, since I suspect people will grep or use their code editors to navigate through these files instead of opening them up one-by-one. I also consider having less structs than the number of files to split them to be a corner case, which is why I think a flag is not necessarily worth the maintenance effort.

The upstream is at openconfig/ygnmi, I think the code changes should be similar since this part of the code is copied. Whenever ygnmi is ready to be widely used (gated mostly on generics stability and various tooling support) then I think that is an appropriate time we can remove ypathgen here.

[wenovus]

Generics isn't actually being used in ygnmi's path struct generation package, so I think we can remove ypathgen now. Filed https://github.com/openconfig/ygot/issues/720

Just verified that we can actually have circular imports between repositories, it's only a circular package import that's disallowed. So when we're remove ypathgen, we can keep the path generation in generator.go if we want, and simply use ygnmi's pathgen code for generation. Alternatively we remove it altogether.

@robshakir I'd actually prefer removing ypathgen's package and code generation altogether from ygot because I don't like seeing two different places where path structs are being generated, and ygot would have to continually get the latest version of ygnmi to keep the generation up-to-date. What do you think.

[robshakir]

That sounds great to me :-)

Do we know all of the callers of generator.go that are doing path struct generation today? I'm not sure that we do - so it might be safer to keep generator.go doing this, but I'm also happy to break this.

In that case, I should take this code and submit it upstream at ypathgen, right?

[wenovus]

That sounds great to me :-)

Do we know all of the callers of generator.go that are doing path struct generation today? I'm not sure that we do - so it might be safer to keep generator.go doing this, but I'm also happy to break this.

In that case, I should take this code and submit it upstream at ypathgen, right?

Yep submitting upstream instead SGTM. I'm thinking we can remove it some point before the end of the year, latest by v1 release. I added the issue to the release milestone.