Infer output:package from output:file #181
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #181 +/- ##
==========================================
- Coverage 95.92% 95.52% -0.41%
==========================================
Files 49 49
Lines 3144 3215 +71
==========================================
+ Hits 3016 3071 +55
- Misses 101 112 +11
- Partials 27 32 +5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks for the contribution! It does work, but I've some remarks that remove the need for doing another packages.Load.
I think we do need to have another packages.Load as the output:file might point to a directory that we haven't loaded yet.
I've worked in some caching in d79e0bb which prevents double loading of files we've already loaded.
| @@ -0,0 +1,43 @@ | |||
| input: | |||
There was a problem hiding this comment.
This test does already exist https://github.com/jmattheis/goverter/blob/main/scenario/extend_external.yml
| if pkg, ok := g.lookupAbsDir[absDir]; ok { | ||
| return pkg.Name, pkg.PkgPath, nil | ||
| } | ||
| // Skipping build tags as they're not used when only using packages.NeedName |
There was a problem hiding this comment.
I think it should use the build tags, because this can produce some flaky edge-cases which should happen less with the build tags.
- someone uses goverter gen with specified output:package
- has the file locally
- removes local output:package
- goverter gen will still use the old package name
- another uses checks out the repo, and the generated file would get a different package.
It doesn't feel right, when the generated file can affect the goverter generation.
| if !filepath.IsAbs(c.OutputFile) { | ||
| absOutputDir = filepath.Join( | ||
| filepath.Dir(rawConverter.FileName), | ||
| filepath.Dir(c.OutputFile)) |
There was a problem hiding this comment.
The output:file may include @cwd to reference the current working directory. this has to be added here too.
| lookup: map[string]*packages.Package{}, | ||
| locals: map[string]map[string]method.LocalOpts{}, | ||
| lookupPkgPath: map[string]*packages.Package{}, | ||
| lookupAbsDir: map[string]*packages.Package{}, |
There was a problem hiding this comment.
Sorry, it took a little longer to review, I had the idea in mind to reuse the existing package loading for this feature, as it is quite similar. There is already this feature to have relative paths in goverter:extend https://github.com/jmattheis/goverter/blob/main/scenario/extend_external_relative.yml, and the package detection could work similarly by detecting a relative path, and joining it with the conversion package path.
It had some rough edges but the end result looks good IMO what do you think? main...alternative this only loads the package for the package name and constructs the package path via path.join.
There is one thing that's not supported, getting the package name of a package outside of the go module goverter is executed in. But IMO this isn't bad, as the normal go tooling works the same. e.g.
$ cd goverter
$ go run ../../gotify/server
directory ../../gotify/server outside main module or its selected dependencies
Heyo! I wanted to try looking into the code base.
I started coding this before I saw your response on #180 (comment). But I'm wondering how well this implementation compares with how you envision this feature.
As I'm brand new to this code base then you probably have some ideas on how to do this better :)
Closes #180