added update_wrapper to NotMemorizedFunc.__init__ #1358
Conversation
Codecov ReportBase: 94.02% // Head: 93.95% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1358 +/- ##
==========================================
- Coverage 94.02% 93.95% -0.07%
==========================================
Files 52 52
Lines 7292 7298 +6
==========================================
+ Hits 6856 6857 +1
- Misses 436 441 +5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
This looks like a worthy change.
I think this was originally not done to make the code of not memorized as light as possible but I think it make things more explicit so I would still do the change.
In order to have the same wrapping behavior, I suggest to make a small wrapping helper and call it in both cases?
| try: | ||
| functools.update_wrapper(self, func) | ||
| except: | ||
| "Objects like ufunc don't like that" |
There was a problem hiding this comment.
Could you put the wrapping code in an helper and use it for both NotMemorizedFunc and MemorizedFunc?
Also, could you also set the doc string of the function, as it is done for MemorizedFunc?
| @@ -344,6 +344,10 @@ class NotMemorizedFunc(object): | |||
| # Should be a light as possible (for speed) | |||
| def __init__(self, func): | |||
| self.func = func | |||
| try: | |||
| functools.update_wrapper(self, func) | |||
| except: | |||
There was a problem hiding this comment.
To avoid the linting error, could you use except BaseException:
Quick fix for a problem I recently found. If no cache is used function wrapped with
@memory.cacheis replaced by NotMemorizedFunc wrapper. This wrapper does nor preserve original properties of the function - in my case name was not available. This makes function behave differently depending whether the cache is defined or not.Code to fix the problem is a copy paste from https://github.com/p610/joblib/blob/master/joblib/memory.py#L444
Maybe there is a better way to address it.