-
Notifications
You must be signed in to change notification settings - Fork 414
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 check_call_in_cache method to check cache without calling function (as per note in the code). Add some tests. #820
Conversation
Codecov Report
@@ Coverage Diff @@
## master #820 +/- ##
==========================================
+ Coverage 94.44% 94.48% +0.04%
==========================================
Files 47 47
Lines 6964 6980 +16
==========================================
+ Hits 6577 6595 +18
+ Misses 387 385 -2
Continue to review full report at Codecov.
|
Code coverage is showing changes in parallel_backends and logger for some reason. No changes there. |
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.
LGTM up to a typo.
Could you please add an entry in the change log?
Just pushed the wrong branch in case someone was about to do something here ... fixing in one sec. |
Squashed again to one commit. And import removal had crept in and the test had not picked it up. I think the risks are now small as it is only one screen of new lines to view in the single commit. |
It seems to me that this PR is addressing the same problem as #730. Did you have a look at it first ? |
nope. 20 line diff. can't even remember this. Whoever is managing this repo just chuck whatever is easiest. Nobody getting paid here for this so no hard feelings :) as long and the functionality gets in. |
This PR seems to have stalled. I think this would be useful. |
@tomMoral just saw this ... I've added something to the doc and the changelog. I'm assuming the pipeline does the auto gen part of the doc stuff. Rebased on master. |
Not sure about those test failures, likely the master I rebased on. I don't think those failures happened before the rebase. |
What happened? Why this is not merged? |
…n (as per note in the code). Add some tests.
I did not notice the update from @cottrell , sorry! |
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.
Some nitpicks.
…ction (joblib#820) Co-authored-by: cottrell <[email protected]> Co-authored-by: Olivier Grisel <[email protected]> Co-authored-by: Thomas Moreau <[email protected]>
No description provided.