-
Notifications
You must be signed in to change notification settings - Fork 21
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
🌱 Update the internal solver before exporting #164
🌱 Update the internal solver before exporting #164
Conversation
Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #164 +/- ##
==========================================
+ Coverage 63.90% 64.43% +0.53%
==========================================
Files 11 11
Lines 482 478 -4
==========================================
Hits 308 308
+ Misses 155 152 -3
+ Partials 19 18 -1 ☔ View full report in Codecov by Sentry. |
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.
These changes look good to me. That being said, I'm not well versed with the entire system and interactions in deppy so another approval is probably good to have prior to merging (I don't think my approval actually counts for merging in this repo anyways)
} | ||
|
||
return nil, ErrIncomplete | ||
return nil, errors.New("cancelled before a solution could be found") |
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.
Is it possible to get here?
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'm not sure. My guess is that Deppy used to accept context for some reason (maybe somehow related to entity sources). So this is probably a leftover from the times where some call coud be cancelled.
I did a bit of code archaeology with a goal to find out the origin of this on Friday, but without any results. I decided to leave this error for now, but removed exported ErrIncomplete
. I think for the scope of this PR it is good enough. But I will dig deeper into this (will create an issue to track this). Edit: see #167
This PR is definitely a step in the right direction. We can resolve whether to use a context or not later. |
Related to #161
Preparing the internal solver to be exported and used without the
DeppySolver
wrapper.