-
Notifications
You must be signed in to change notification settings - Fork 46
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
✨ add dependency location resolver and get code locations for XML, JSON files #412
Conversation
@shawn-hurley @fabianvf @djzager Let me know what you guys think of this approach |
429f946
to
c41efdb
Compare
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 looks good to me, did have a couple of concerns but overall I like the cache implementation
0be9c98
to
bcb3843
Compare
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.
Looks good except for the one question around float64 and int
@@ -128,6 +128,8 @@ func (p *builtinProvider) Init(ctx context.Context, log logr.Logger, config prov | |||
config: config, | |||
tags: p.tags, | |||
UnimplementedDependenciesComponent: provider.UnimplementedDependenciesComponent{}, | |||
locationCache: make(map[string]float64), |
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 float and not int?
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.
For some reason, in provider API, the value is float
Looks good! |
2400be0
to
667ff88
Compare
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
02b98ae
to
4f3b1f2
Compare
Fixes #410
Fixes #254
There were two choices to do this:
I chose No. 2 because output API change not needed. But I am not fan of it as its complicated & perf penalty. Asking for your inputs on the approach. If we do go with 2, I think we should we should still remove it in later versions in favor of a new optional field on Dep. Even if we did add it now, it would be non-intrusive.