From 5256eae60d3685de51c1f3891abe157e15d14def Mon Sep 17 00:00:00 2001 From: Eugenio Lacuesta Date: Thu, 7 May 2020 14:37:41 -0300 Subject: [PATCH] Meta class to handle isinstance checks for BaseItem --- pytest.ini | 2 +- scrapy/commands/parse.py | 4 +-- scrapy/contracts/default.py | 8 +++--- scrapy/core/scraper.py | 4 +-- scrapy/exporters.py | 4 +-- scrapy/item.py | 20 +++++++++++-- scrapy/shell.py | 5 ++-- scrapy/utils/misc.py | 4 +-- scrapy/utils/serialize.py | 4 +-- tests/test_item.py | 56 +++++++++++++++++++++++++++++++++---- 10 files changed, 85 insertions(+), 26 deletions(-) diff --git a/pytest.ini b/pytest.ini index 5a86ce2a7..292dbce41 100644 --- a/pytest.ini +++ b/pytest.ini @@ -204,7 +204,7 @@ flake8-ignore = tests/test_http_headers.py E501 tests/test_http_request.py E402 E501 E127 E128 E128 E126 E123 tests/test_http_response.py E501 E128 - tests/test_item.py E128 F841 + tests/test_item.py E128 F841 E501 tests/test_link.py E501 tests/test_linkextractors.py E501 E128 E124 tests/test_loader.py E501 E741 E128 E117 diff --git a/scrapy/commands/parse.py b/scrapy/commands/parse.py index 1cefed106..098827ab9 100644 --- a/scrapy/commands/parse.py +++ b/scrapy/commands/parse.py @@ -5,7 +5,7 @@ from w3lib.url import is_url from scrapy.commands import ScrapyCommand from scrapy.http import Request -from scrapy.item import BaseItem +from scrapy.item import _BaseItem from scrapy.utils import display from scrapy.utils.conf import arglist_to_dict from scrapy.utils.spider import iterate_spider_output, spidercls_for_request @@ -117,7 +117,7 @@ class Command(ScrapyCommand): items, requests = [], [] for x in iterate_spider_output(callback(response, **cb_kwargs)): - if isinstance(x, (BaseItem, dict)): + if isinstance(x, (_BaseItem, dict)): items.append(x) elif isinstance(x, Request): requests.append(x) diff --git a/scrapy/contracts/default.py b/scrapy/contracts/default.py index a1b0f8f22..cdc2bac15 100644 --- a/scrapy/contracts/default.py +++ b/scrapy/contracts/default.py @@ -1,6 +1,6 @@ import json -from scrapy.item import BaseItem +from scrapy.item import _BaseItem from scrapy.http import Request from scrapy.exceptions import ContractFail @@ -51,8 +51,8 @@ class ReturnsContract(Contract): objects = { 'request': Request, 'requests': Request, - 'item': (BaseItem, dict), - 'items': (BaseItem, dict), + 'item': (_BaseItem, dict), + 'items': (_BaseItem, dict), } def __init__(self, *args, **kwargs): @@ -103,7 +103,7 @@ class ScrapesContract(Contract): def post_process(self, output): for x in output: - if isinstance(x, (BaseItem, dict)): + if isinstance(x, (_BaseItem, dict)): missing = [arg for arg in self.args if arg not in x] if missing: raise ContractFail( diff --git a/scrapy/core/scraper.py b/scrapy/core/scraper.py index edbb4dd66..6785e103d 100644 --- a/scrapy/core/scraper.py +++ b/scrapy/core/scraper.py @@ -14,7 +14,7 @@ from scrapy.utils.log import logformatter_adapter, failure_to_exc_info from scrapy.exceptions import CloseSpider, DropItem, IgnoreRequest from scrapy import signals from scrapy.http import Request, Response -from scrapy.item import BaseItem +from scrapy.item import _BaseItem from scrapy.core.spidermw import SpiderMiddlewareManager @@ -191,7 +191,7 @@ class Scraper: """ if isinstance(output, Request): self.crawler.engine.crawl(request=output, spider=spider) - elif isinstance(output, (BaseItem, dict)): + elif isinstance(output, (_BaseItem, dict)): self.slot.itemproc_size += 1 dfd = self.itemproc.process_item(output, spider) dfd.addBoth(self._itemproc_finished, output, response, spider) diff --git a/scrapy/exporters.py b/scrapy/exporters.py index 0cb6cef98..4731b925a 100644 --- a/scrapy/exporters.py +++ b/scrapy/exporters.py @@ -12,7 +12,7 @@ from xml.sax.saxutils import XMLGenerator from scrapy.utils.serialize import ScrapyJSONEncoder from scrapy.utils.python import to_bytes, to_unicode, is_listlike -from scrapy.item import BaseItem +from scrapy.item import _BaseItem from scrapy.exceptions import ScrapyDeprecationWarning @@ -312,7 +312,7 @@ class PythonItemExporter(BaseItemExporter): return serializer(value) def _serialize_value(self, value): - if isinstance(value, BaseItem): + if isinstance(value, _BaseItem): return self.export_item(value) if isinstance(value, dict): return dict(self._serialize_dict(value)) diff --git a/scrapy/item.py b/scrapy/item.py index 46d20d017..f468ff86f 100644 --- a/scrapy/item.py +++ b/scrapy/item.py @@ -14,7 +14,23 @@ from scrapy.utils.deprecate import ScrapyDeprecationWarning from scrapy.utils.trackref import object_ref -class BaseItem(object_ref): +class _BaseItem(object_ref): + """ + Temporary class used internally to avoid the deprecation + warning raised by isinstance checks using BaseItem. + """ + pass + + +class _BaseItemMeta(ABCMeta): + def __instancecheck__(cls, instance): + if cls is BaseItem: + warn('scrapy.item.BaseItem is deprecated, please use scrapy.item.Item instead', + ScrapyDeprecationWarning, stacklevel=2) + return super().__instancecheck__(instance) + + +class BaseItem(_BaseItem, metaclass=_BaseItemMeta): """ Deprecated, please use :class:`scrapy.item.Item` instead """ @@ -30,7 +46,7 @@ class Field(dict): """Container of field metadata""" -class ItemMeta(ABCMeta): +class ItemMeta(_BaseItemMeta): """Metaclass_ of :class:`Item` that handles field definitions. .. _metaclass: https://realpython.com/python-metaclasses diff --git a/scrapy/shell.py b/scrapy/shell.py index 08ce89481..83afb74c9 100644 --- a/scrapy/shell.py +++ b/scrapy/shell.py @@ -13,7 +13,7 @@ from w3lib.url import any_to_uri from scrapy.crawler import Crawler from scrapy.exceptions import IgnoreRequest from scrapy.http import Request, Response -from scrapy.item import BaseItem +from scrapy.item import _BaseItem from scrapy.settings import Settings from scrapy.spiders import Spider from scrapy.utils.console import start_python_console @@ -26,8 +26,7 @@ from scrapy.utils.console import DEFAULT_PYTHON_SHELLS class Shell: - relevant_classes = (Crawler, Spider, Request, Response, BaseItem, - Settings) + relevant_classes = (Crawler, Spider, Request, Response, _BaseItem, Settings) def __init__(self, crawler, update_vars=None, code=None): self.crawler = crawler diff --git a/scrapy/utils/misc.py b/scrapy/utils/misc.py index 52cfba208..bfe3ccd40 100644 --- a/scrapy/utils/misc.py +++ b/scrapy/utils/misc.py @@ -14,10 +14,10 @@ from w3lib.html import replace_entities from scrapy.utils.datatypes import LocalWeakReferencedCache from scrapy.utils.python import flatten, to_unicode -from scrapy.item import BaseItem +from scrapy.item import _BaseItem -_ITERABLE_SINGLE_VALUES = dict, BaseItem, str, bytes +_ITERABLE_SINGLE_VALUES = dict, _BaseItem, str, bytes def arg_to_iter(arg): diff --git a/scrapy/utils/serialize.py b/scrapy/utils/serialize.py index 9dd72ea71..bf73dfa18 100644 --- a/scrapy/utils/serialize.py +++ b/scrapy/utils/serialize.py @@ -5,7 +5,7 @@ import decimal from twisted.internet import defer from scrapy.http import Request, Response -from scrapy.item import BaseItem +from scrapy.item import _BaseItem class ScrapyJSONEncoder(json.JSONEncoder): @@ -26,7 +26,7 @@ class ScrapyJSONEncoder(json.JSONEncoder): return str(o) elif isinstance(o, defer.Deferred): return str(o) - elif isinstance(o, BaseItem): + elif isinstance(o, _BaseItem): return dict(o) elif isinstance(o, Request): return "<%s %s %s>" % (type(o).__name__, o.method, o.url) diff --git a/tests/test_item.py b/tests/test_item.py index f35a2b9f9..6fdd7e302 100644 --- a/tests/test_item.py +++ b/tests/test_item.py @@ -4,7 +4,7 @@ from unittest import mock from warnings import catch_warnings from scrapy.exceptions import ScrapyDeprecationWarning -from scrapy.item import ABCMeta, BaseItem, DictItem, Field, Item, ItemMeta +from scrapy.item import ABCMeta, _BaseItem, BaseItem, DictItem, Field, Item, ItemMeta PY36_PLUS = (sys.version_info.major >= 3) and (sys.version_info.minor >= 6) @@ -334,29 +334,73 @@ class DictItemTest(unittest.TestCase): class BaseItemTest(unittest.TestCase): + def test_isinstance_check(self): + + class SubclassedBaseItem(BaseItem): + pass + + class SubclassedItem(Item): + pass + + self.assertTrue(isinstance(BaseItem(), BaseItem)) + self.assertTrue(isinstance(SubclassedBaseItem(), BaseItem)) + self.assertTrue(isinstance(Item(), BaseItem)) + self.assertTrue(isinstance(SubclassedItem(), BaseItem)) + + # make sure internal checks using private _BaseItem class succeed + self.assertTrue(isinstance(BaseItem(), _BaseItem)) + self.assertTrue(isinstance(SubclassedBaseItem(), _BaseItem)) + self.assertTrue(isinstance(Item(), _BaseItem)) + self.assertTrue(isinstance(SubclassedItem(), _BaseItem)) + def test_deprecation_warning(self): + """ + Make sure deprecation warnings are logged whenever BaseItem is used, + either instantiated or in an isinstance check + """ with catch_warnings(record=True) as warnings: BaseItem() self.assertEqual(len(warnings), 1) self.assertEqual(warnings[0].category, ScrapyDeprecationWarning) + with catch_warnings(record=True) as warnings: + class SubclassedBaseItem(BaseItem): pass + SubclassedBaseItem() self.assertEqual(len(warnings), 1) self.assertEqual(warnings[0].category, ScrapyDeprecationWarning) + with catch_warnings(record=True) as warnings: + self.assertFalse(isinstance("foo", BaseItem)) + self.assertEqual(len(warnings), 1) + self.assertEqual(warnings[0].category, ScrapyDeprecationWarning) + + with catch_warnings(record=True) as warnings: + self.assertTrue(isinstance(BaseItem(), BaseItem)) + self.assertEqual(len(warnings), 1) + self.assertEqual(warnings[0].category, ScrapyDeprecationWarning) + class ItemNoDeprecationWarningTest(unittest.TestCase): - def test_no_deprecation_warning(self): + """ + Make sure deprecation warnings are NOT logged whenever BaseItem subclasses are used. + """ + class SubclassedItem(Item): + pass + with catch_warnings(record=True) as warnings: Item() - self.assertEqual(len(warnings), 0) - with catch_warnings(record=True) as warnings: - class SubclassedItem(Item): - pass SubclassedItem() + _BaseItem() + self.assertFalse(isinstance("foo", _BaseItem)) + self.assertFalse(isinstance("foo", Item)) + self.assertFalse(isinstance("foo", SubclassedItem)) + self.assertTrue(isinstance(_BaseItem(), _BaseItem)) + self.assertTrue(isinstance(Item(), Item)) + self.assertTrue(isinstance(SubclassedItem(), SubclassedItem)) self.assertEqual(len(warnings), 0)