-
Notifications
You must be signed in to change notification settings - Fork 12
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(injector): source import shadowed results in build error "X is not a type" #463
Conversation
24d3523
to
6e5668a
Compare
Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
6e5668a
to
cb6d841
Compare
nodeChain := []dst.Node{c.node} | ||
for p := c.NodeChain.parent; p != nil; p = p.parent { | ||
nodeChain = append(nodeChain, p.node) | ||
} | ||
|
||
return c.refMap.AddImport(c.file, nodeChain, path, name) |
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.
Why not just pass the c.NodeChain
? That would be equivalent, but incur fewer allocations? (Here you're basically copying a linked list into an array list...)
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.
Because of circular imports :/
@@ -16,6 +16,7 @@ aspects: | |||
}() | |||
|
|||
syntheticReferences: | |||
github.com/go-chi/chi/v5: true |
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.
That reference exists in the original file, so it should be be reported as a synthetic one? It's not a big deal here (it'd be filtered later one due to it already being present in the importcfg
file, but still...)
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.
This is part of the idea to mitigate shadowing when dot imports are used
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #463 +/- ##
==========================================
+ Coverage 57.02% 63.96% +6.94%
==========================================
Files 176 184 +8
Lines 10384 11084 +700
==========================================
+ Hits 5921 7090 +1169
+ Misses 4021 3461 -560
- Partials 442 533 +91
|
What
Previously we would only check if the import was present in a fix. Now, we are checking if import is accessible as the import we are looking for.
References
Fixes #458
Fixes #448