Refactor how path structs are split. #719
Conversation
robshakir
commented
Aug 5, 2022
robshakir
commented
* (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.
There was a problem hiding this comment.
So the code looks good to me, though I am wondering if we should continue to generate the empty files in order to match whatever number of files the user specified? The reason being for build systems like bazel AFAIK there isn't an easy way to deal with an unknown number of generated files, and if you specify a file that isn't generated the build system will complain. Empty files seem to be mostly harmless to me, so I think the benefits of keeping them outweigh the negatives. What do you think?
| if i != 0 && i%structsPerFile == 0 { | ||
| fileContent = append(fileContent, structs) | ||
| structs = []string{} | ||
| } | ||
| structs = append(structs, gotStruct.String()) | ||
| } | ||
| for i := 0; i != emptyFiles; i++ { | ||
| files = append(files, genCode.CommonHeader) | ||
| fileContent = append(fileContent, structs) |
There was a problem hiding this comment.
(optional): Move line 410 to just before line 406, change if condition to if i != 0 && (i%structsPerFile == 0 || i == structN-1) {, and you can remove the corner case after the for loop (delete line 412)
|
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. |
|
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. |
|
Generics isn't actually being used in ygnmi's path struct generation package, so I think we can remove 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 @robshakir I'd actually prefer removing |
|
That sounds great to me :-) Do we know all of the callers of 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. |
