-
Notifications
You must be signed in to change notification settings - Fork 54
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
🌱 Part 5: Reduce number of variable sources. Final cleanup #501
🌱 Part 5: Reduce number of variable sources. Final cleanup #501
Conversation
8a23cd9
to
0b2a515
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #501 +/- ##
==========================================
- Coverage 85.30% 83.69% -1.61%
==========================================
Files 23 20 -3
Lines 898 822 -76
==========================================
- Hits 766 688 -78
- Misses 91 92 +1
- Partials 41 42 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
0b2a515
to
27dd463
Compare
27dd463
to
7cee1e6
Compare
8e17e8e
to
94024b3
Compare
94024b3
to
f564436
Compare
81ea29a
to
420d35a
Compare
420d35a
to
ef98071
Compare
Signed-off-by: Mikalai Radchuk <[email protected]>
Signed-off-by: Mikalai Radchuk <[email protected]>
ef98071
to
c73b5a6
Compare
|
||
bundleUniqueness := variablesources.MakeBundleUniquenessVariables(bundles) | ||
|
||
result := []deppy.Variable{} |
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.
Maybe take a look at https://freshman.tech/snippets/go/concatenate-slices/, specifically the section "Concatenating multiple slices at once", as a way to do this more efficiently?
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.
@ncdc the above works when we want to concatenate slices of the same type. Here we have slices of multiple different types all of which implement deppy.Variable
interface and we need to create a slice of deppy.Variable
s. I don't think it will work here.
I considered doing pre-allocation like this:
result := make([]deppy.Variable, 0, len(requiredPackages)+len(installedPackages)+len(bundles)+len(bundleUniqueness))
But decided not to go with it. It still requires a loop with an append
. Also It seems brittle: I can definitely see myself not updating the make
call with a new variable when adding another loop. But it avoids extra allocations.
6222ad1
Description
Spliting #460 into smaller chunks.
In this PR I replace controller variable source with an implementation containing a bunch of function calls (from parts 1-4). I also remove old variable sources and related tests
Closes #437
Reviewer Checklist