⚠️ Remove variable sources and make Deppy accept a slice of variables instead #160
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #160 +/- ##
==========================================
- Coverage 65.52% 63.90% -1.63%
==========================================
Files 11 11
Lines 496 482 -14
==========================================
- Hits 325 308 -17
- Misses 152 155 +3
Partials 19 19 ☔ View full report in Codecov by Sentry. |
| // Solution is returned by the Solver when the internal solver executed successfully. | ||
| // A successful execution of the solver can still end in an error when no solution can | ||
| // be found. | ||
| type Solution struct { | ||
| err error | ||
| selection map[deppy.Identifier]deppy.Variable | ||
| variables []deppy.Variable | ||
| } |
There was a problem hiding this comment.
I think we can get rid of Solution completely (not in this PR). I do not see a lot of value in it.
And probably replace DeppySolver wrapper around internal package by exporting what we currently have in internal pacakge (with some modifications and also not in this PR).
This comment is for me to look into whether it makes sense and create issues for this.
Edit: created #161 for this
There was a problem hiding this comment.
Structuring the logic to be helpers in generating the inputs to and consuming the outputs of the solver library sounds like a really good idea - lets folks use as much of or as little of this helpful logic as they need and doesn't bind them to what we happen to have or not to have wrapped at some moment in time.
m1kola
force-pushed
the
remove_variable_sources
branch
from
f8dc17f to
7cf6aaa
Compare
| if solutionOpts.addVariablesToSolution { | ||
| solution.variables = vars | ||
| } |
There was a problem hiding this comment.
Previously variables were generated by a varaible soruce. So for cases where the calling code for some reason needed to access input variables - we had an option to add them into the Solution struct. Since now the calling code provides input variables - this seems like pointless option. So I removed it.
| @@ -141,26 +116,6 @@ var _ = Describe("Entity", func() { | |||
| })) | |||
| }) | |||
|
|
|||
| It("should add all variables to solution if option is given", func() { | |||
There was a problem hiding this comment.
No longer have this option.
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(solution.Error()).To(HaveOccurred()) | ||
| }) | ||
|
|
||
| It("should return peripheral errors", func() { |
There was a problem hiding this comment.
No longer have a variable soruce to fail
|
|
openshift-merge-robot
added
the
needs-rebase
Signed-off-by: Mikalai Radchuk <[email protected]>
m1kola
force-pushed
the
remove_variable_sources
branch
from
7cf6aaa to
e027f4d
Compare
openshift-merge-robot
removed
the
needs-rebase
m1kola
force-pushed
the
remove_variable_sources
branch
from
e027f4d to
517368d
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
m1kola
changed the title
Instead of a variable source make Deppy accept a slice of variables as an input. Signed-off-by: Mikalai Radchuk <[email protected]>
m1kola
force-pushed
the
remove_variable_sources
branch
from
517368d to
792c624
Compare
There was a problem hiding this comment.
I think these changes look great and will make it easier to use, thanks @m1kola !
af7d9eb
Instead of a variable source make Deppy accept a slice of variables as an input.
Closes #153