Code cleanup scrapy.utils.python.WeakKeyCache #4684 #4701
Conversation
|
Cleaning up scrapy.utils.python.WeakKeyCache code and tests as per #4684 |
|
Flake8 failed, can you please fix that? |
|
@wRAR : The failures don't seem to be related to my change can you confirm, please? |
|
Indeed. @Gallaecio are these some known intermittent failures? |
|
@wRAR : Can I try pushing in an empty commit ? |
Codecov Report
@@ Coverage Diff @@
## master #4701 +/- ##
==========================================
+ Coverage 86.29% 86.33% +0.03%
==========================================
Files 160 160
Lines 9644 9672 +28
Branches 1415 1419 +4
==========================================
+ Hits 8322 8350 +28
Misses 1062 1062
Partials 260 260
|
|
@wRAR The indeed seem to be intermittent errors. Pushing an empty commit fixed it. |
|
I've re-read the original issue, it talks about deprecating the class, not removing it, we don't remove non-deprecated classes/functions most of the time, can you please mark it as deprecated instead? |
|
@wRAR : Would we like to bring back the test as well ? |
|
No, I think it's fine to not test it as it's currently not used. |
|
Made the changes as requested |
scrapy/utils/python.py
Outdated
| @@ -273,6 +275,7 @@ def equal_attributes(obj1, obj2, attributes): | |||
| return True | |||
|
|
|||
|
|
|||
| @deprecated | |||
There was a problem hiding this comment.
I don't think it works on classes? Have you tried to instantiate it?
There was a problem hiding this comment.
@wRAR Yep, this is working with classes as inherently we are calling the __init__ function .
I tested it out and it gave me the following warning :
UserWarning: Call to deprecated function WeakKeyCache. Use The following API is deprecated Use instead.
There was a problem hiding this comment.
Well, the message is inaccurate, “deprecated function WeakKeyCache”. We need to either make @deprecated smart enough to say ‘class’ when needed, or use a different approach. We’ve been using the latter so far, I’m not sure if there are potential unexpected side effects of using @deprecated after changing it to say “class” for classes.
There was a problem hiding this comment.
@Gallaecio @wRAR What is the latter approach which has been used so far?
Also which is the existing test function?
There was a problem hiding this comment.
@Gallaecio @wRAR What is the latter approach which has been used so far?
Logging a warning manually in the __init__ method, with ScrapyDeprecationWarning as warning class, and stacklevel=2 (so that the warning is about the call to __init__, and not about the implementation line where we log the warning).
Also which is the existing test function?
I did not get this, sorry. Could you elaborate?
Actually, since the point of deprecating instead of removing is for things to remain working as expected for a while, I think it is better for the test to stay until we remove the class itself. Also, it would be great to include a test that verifies that using the class logs the expected deprecation warning, or modify the existing test accordingly. |
|
@Gallaecio @wRAR : Do we follow the principle of like deprecate in release and remove in subsequent release? |
Nope. We only recently discussed internally our deprecation policy, and we are adding it to the documentation soon: deprecated code continues working for at least 1 year. |
|
@wRAR @Gallaecio I went ahead with latter change as there doesn't seem to be reliable way to differentiate between a class/method. Kindly review. |
|
Thank you! |
Resolves #4684