feat(maker-cross-fil-types): ifacemaker type resolution across multi-file packages#112
feat(maker-cross-fil-types): ifacemaker type resolution across multi-file packages#112zdunecki wants to merge 4 commits intovburenin:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances ifacemaker's type resolution capability to discover embedded struct types across multiple files within the same package. Previously, ifacemaker only examined the explicitly provided input files, which caused issues when type declarations (like sqlc-generated models) were split across separate files from their method implementations. The solution scans all Go files in the same directories as the input files to build a complete picture of available types and embedding relationships.
Changes:
- Modified
Make()function to scan entire directories for type declarations and embedding graphs, not just explicitly provided files - Added fixture-based testing infrastructure with JSON configuration files
- Introduced deduplication using a
parsedFilesmap to avoid processing the same file multiple times
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| maker/maker.go | Core logic changes: added directory scanning, absolute path handling, and parseAndCollect closure to discover types across all Go files in same directories |
| ifacemaker_test.go | New fixture-based testing infrastructure with fixtureConfig struct and TestFixtures function |
| go.mod / go.sum | Added test dependencies (kr/pretty, rogpeppe/go-internal, check.v1) |
| fixtures/1.nested-modules/* | Complete fixture demonstrating cross-file type resolution with pet store example spanning multiple packages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@zdunecki The unit-tests are causing random failures related to the changes in maker.go |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dir := filepath.Dir(absPath) | ||
| entries, err := os.ReadDir(dir) | ||
| if err != nil { | ||
| return []byte{}, err | ||
| } |
There was a problem hiding this comment.
When multiple input files are in the same directory, this implementation will call os.ReadDir on that directory multiple times (once per input file in that directory). While the parsedFiles map prevents re-parsing the same file, the directory listing itself is repeated unnecessarily. Consider collecting unique directories first, or caching directory listings to reduce redundant filesystem operations.
| absPath := f | ||
| if !filepath.IsAbs(absPath) { | ||
| if p, err := filepath.Abs(absPath); err != nil { | ||
| log.Printf("maker: failed to convert %q to absolute path: %v", absPath, err) | ||
| } else { | ||
| absPath = p | ||
| } |
There was a problem hiding this comment.
The file deduplication using parsedFiles map may not work correctly if the same physical file is referenced through different paths (e.g., relative vs absolute, or through symlinks). Consider normalizing all paths to absolute paths before using them as map keys, and handle the error case where filepath.Abs fails more robustly (e.g., by returning an error rather than just logging and continuing with a potentially non-normalized path).
|
@gaby I'll look what's going on with failures soon. |
2 participants
Hi,
This PR allows to find embedded structs more deeply. For example if we import structs from two packages of same module and those have separted models and implementation in different files (
__fixtures__/1.nested-modules/input).Best,