-
Notifications
You must be signed in to change notification settings - Fork 409
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
fix: address module imports that end with .go #487
Conversation
pkg/generator.go
Outdated
@@ -194,7 +194,7 @@ func (g *Generator) getLocalizedPath(ctx context.Context, path string) string { | |||
log := zerolog.Ctx(ctx).With().Str(logging.LogKeyPath, path).Logger() | |||
ctx = log.WithContext(ctx) | |||
|
|||
if strings.HasSuffix(path, ".go") { | |||
if strings.HasSuffix(path, ".go") && strings.Contains(path, "vendor"+string(filepath.Separator)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain this a little more? Doesn't this break the caching logic if you have any local .go
file that isn't under a vendor directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I think we might break the logic in this case. It looks like I'd need to refactor the logic to ensure the path we want to preserve is only a directory and nothing else. I think we can look at the import struct we have to check
pkg/generator_test.go
Outdated
result := generator.getLocalizedPath(context.Background(), "vendor/github.com/nats-io/nats.go/js.go") | ||
|
||
s.Equal( | ||
"github.com/nats-io/nats.go", result, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're vendoring a directory don't you want the returned path to be on-disk path instead of the URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In golang code, you put imports to a vendored directory in same way as from modules, that's why this current logic is in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I've never personally used vendored dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I used to. It's just a directory with all project dependencies in a form of a source code. In any case, we ought to check if a path element is a directory or not and if it's a file, remove it from path to build correct import statement.
7bc5fef
to
2ca0b83
Compare
@LandonTClipp I think you merged the PR too early, The PR didn't have any changes. I was digging a little bit deeper in the code to understand how imports are generated, taking more time than expected :) |
That's really odd I didn't intend to merge this. I must have fat fingered something! Sorry about that. |
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
.
in them are incorrectly imported in mocks #481 generated mock incorrectly imports package ending in .go #249 Mock generation with NATS leads to import errors (version: 2.7.5) #419Type of change
Version of Golang used when building/testing:
How Has This Been Tested?
See
TestGetLocalizedPathVendored
andTestGetLocalizedPathModule
ingenerator_test.go
Checklist