Repository: scrapy/scrapy
Pull request #4031 (2019-09-22 11:14:46)

remove .keys() to avoid creating a tmp list/keyview obj
This is a small straight forward refactor to avoid creating a list or keyview object ( list in 2.x and key in 3.x ). 
There are other folders where this change can be made but starting with commands folder.

s-sanjay: the test seems to only fail in py35-pinned env... weird..
codecov[bot]: # [Codecov](https://codecov.io/gh/scrapy/scrapy/pull/4031?src=pr&el=h1) Report
> Merging [#4031](https://codecov.io/gh/scrapy/scrapy/pull/4031?src=pr&el=desc) into [master](https://codecov.io/gh/scrapy/scrapy/commit/1364e92007308924cec2ac15cb9a3ec79eb9dc23?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `0%`.


```diff
@@            Coverage Diff             @@
##           master    #4031      +/-   ##
==========================================
+ Coverage   85.67%   85.68%   +<.01%     
==========================================
  Files         165      165              
  Lines        9726     9725       -1     
  Branches     1461     1462       +1     
==========================================
  Hits         8333     8333              
+ Misses       1136     1135       -1     
  Partials      257      257
```

| [Impacted Files](https://codecov.io/gh/scrapy/scrapy/pull/4031?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [scrapy/commands/parse.py](https://codecov.io/gh/scrapy/scrapy/pull/4031/diff?src=pr&el=tree#diff-c2NyYXB5L2NvbW1hbmRzL3BhcnNlLnB5) | `20.21% <0%> (-0.12%)` | :arrow_down: |
| [scrapy/commands/runspider.py](https://codecov.io/gh/scrapy/scrapy/pull/4031/diff?src=pr&el=tree#diff-c2NyYXB5L2NvbW1hbmRzL3J1bnNwaWRlci5weQ==) | `75.36% <0%> (+1.07%)` | :arrow_up: |
| [scrapy/commands/crawl.py](https://codecov.io/gh/scrapy/scrapy/pull/4031/diff?src=pr&el=tree#diff-c2NyYXB5L2NvbW1hbmRzL2NyYXdsLnB5) | `28.57% <0%> (+0.66%)` | :arrow_up: |

s-sanjay: > The 2nd and 3rd changes seem straightforward to me, but I wonder what is the impact of the first change (`max_level`) in performance.

so performance wise we are avoiding creating a dynamic list of size of both the list. 
we loop through both the dict, and take the max and take max of that max. 
earlier we loop through both the dict and create combined new list of keys and then find max in that list.
s-sanjay: @Gallaecio can this be merged ?

Patch:
diff --git a/docs/conf.py b/docs/conf.py
index fa257dead..34dd5bcb7 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -262,6 +262,9 @@ coverage_ignore_pyobjects = [
     # details that are not documented.
     r'^scrapy\.extensions\.[a-z]\w*?\.[A-Z]\w*?\.',  # methods
     r'^scrapy\.extensions\.[a-z]\w*?\.[a-z]',  # helper functions
+
+    # Never documented before, and deprecated now.
+    r'^scrapy\.item\.DictItem$',
 ]
 
 
diff --git a/docs/topics/items.rst b/docs/topics/items.rst
index 60fbc82f8..260f5882c 100644
--- a/docs/topics/items.rst
+++ b/docs/topics/items.rst
@@ -263,3 +263,9 @@ Field objects
 .. _dict: https://docs.python.org/2/library/stdtypes.html#dict
 
 
+Other classes related to Item
+=============================
+
+.. autoclass:: BaseItem
+
+.. autoclass:: ItemMeta
diff --git a/scrapy/commands/parse.py b/scrapy/commands/parse.py
index d4f2234b0..ef8acd29c 100644
--- a/scrapy/commands/parse.py
+++ b/scrapy/commands/parse.py
@@ -60,10 +60,12 @@ class Command(ScrapyCommand):
 
     @property
     def max_level(self):
-        levels = list(self.items.keys()) + list(self.requests.keys())
-        if not levels:
-            return 0
-        return max(levels)
+        max_items, max_requests = 0, 0
+        if self.items:
+            max_items = max(self.items)
+        if self.requests:
+            max_requests = max(self.requests)
+        return max(max_items, max_requests)
 
     def add_items(self, lvl, new_items):
         old_items = self.items.get(lvl, [])
@@ -84,9 +86,8 @@ class Command(ScrapyCommand):
 
     def print_requests(self, lvl=None, colour=True):
         if lvl is None:
-            levels = list(self.requests.keys())
-            if levels:
-                requests = self.requests[max(levels)]
+            if self.requests:
+                requests = self.requests[max(self.requests)]
             else:
                 requests = []
         else:
diff --git a/scrapy/commands/runspider.py b/scrapy/commands/runspider.py
index 376d3c84e..57d8471ca 100644
--- a/scrapy/commands/runspider.py
+++ b/scrapy/commands/runspider.py
@@ -60,14 +60,13 @@ class Command(ScrapyCommand):
             else:
                 self.settings.set('FEED_URI', opts.output, priority='cmdline')
             feed_exporters = without_none_values(self.settings.getwithbase('FEED_EXPORTERS'))
-            valid_output_formats = feed_exporters.keys()
             if not opts.output_format:
                 opts.output_format = os.path.splitext(opts.output)[1].replace(".", "")
-            if opts.output_format not in valid_output_formats:
+            if opts.output_format not in feed_exporters:
                 raise UsageError("Unrecognized output format '%s', set one"
                                  " using the '-t' switch or as a file extension"
                                  " from the supported list %s" % (opts.output_format,
-                                                                  tuple(valid_output_formats)))
+                                                                  tuple(feed_exporters)))
             self.settings.set('FEED_FORMAT', opts.output_format, priority='cmdline')
 
     def run(self, args, opts):
diff --git a/scrapy/item.py b/scrapy/item.py
index 9d4786788..73b8f54b0 100644
--- a/scrapy/item.py
+++ b/scrapy/item.py
@@ -4,13 +4,15 @@ Scrapy Item
 See documentation in docs/topics/item.rst
 """
 
+import collections
 from abc import ABCMeta
-from pprint import pformat
 from copy import deepcopy
-import collections
+from pprint import pformat
+from warnings import warn
 
 import six
 
+from scrapy.utils.deprecate import ScrapyDeprecationWarning
 from scrapy.utils.trackref import object_ref
 
 
@@ -21,7 +23,19 @@ else:
 
 
 class BaseItem(object_ref):
-    """Base class for all scraped items."""
+    """Base class for all scraped items.
+
+    In Scrapy, an object is considered an *item* if it is an instance of either
+    :class:`BaseItem` or :class:`dict`. For example, when the output of a
+    spider callback is evaluated, only instances of :class:`BaseItem` or
+    :class:`dict` are passed to :ref:`item pipelines <topics-item-pipeline>`.
+
+    If you need instances of a custom class to be considered items by Scrapy,
+    you must inherit from either :class:`BaseItem` or :class:`dict`.
+
+    Unlike instances of :class:`dict`, instances of :class:`BaseItem` may be
+    :ref:`tracked <topics-leaks-trackrefs>` to debug memory leaks.
+    """
     pass
 
 
@@ -30,6 +44,10 @@ class Field(dict):
 
 
 class ItemMeta(ABCMeta):
+    """Metaclass_ of :class:`Item` that handles field definitions.
+
+    .. _metaclass: https://realpython.com/python-metaclasses
+    """
 
     def __new__(mcs, class_name, bases, attrs):
         classcell = attrs.pop('__classcell__', None)
@@ -56,6 +74,13 @@ class DictItem(MutableMapping, BaseItem):
 
     fields = {}
 
+    def __new__(cls, *args, **kwargs):
+        if issubclass(cls, DictItem) and not issubclass(cls, Item):
+            warn('scrapy.item.DictItem is deprecated, please use '
+                 'scrapy.item.Item instead',
+                 ScrapyDeprecationWarning, stacklevel=2)
+        return super(DictItem, cls).__new__(cls, *args, **kwargs)
+
     def __init__(self, *args, **kwargs):
         self._values = {}
         if args or kwargs:  # avoid creating dict for most common case
diff --git a/scrapy/utils/request.py b/scrapy/utils/request.py
index 50bc3cb1e..9c143b83a 100644
--- a/scrapy/utils/request.py
+++ b/scrapy/utils/request.py
@@ -30,7 +30,7 @@ def request_fingerprint(request, include_headers=None):
     and are equivalent (ie. they should return the same response).
 
     Another example are cookies used to store session ids. Suppose the
-    following page is only accesible to authenticated users:
+    following page is only accessible to authenticated users:
 
     http://www.example.com/members/offers.html
 


Test patch:
diff --git a/tests/test_command_parse.py b/tests/test_command_parse.py
index 98e415ad3..62d5d76b4 100644
--- a/tests/test_command_parse.py
+++ b/tests/test_command_parse.py
@@ -108,6 +108,7 @@ ITEM_PIPELINES = {'%s.pipelines.MyPipeline': 1}
         _, _, stderr = yield self.execute(['--spider', self.spider_name,
                                            '-a', 'test_arg=1',
                                            '-c', 'parse',
+                                           '--verbose',
                                            self.url('/html')])
         self.assertIn("DEBUG: It Works!", _textmode(stderr))
 
@@ -117,12 +118,14 @@ ITEM_PIPELINES = {'%s.pipelines.MyPipeline': 1}
         _, _, stderr = yield self.execute(['--spider', self.spider_name,
                                            '--meta', raw_json_string,
                                            '-c', 'parse_request_with_meta',
+                                           '--verbose',
                                            self.url('/html')])
         self.assertIn("DEBUG: It Works!", _textmode(stderr))
 
         _, _, stderr = yield self.execute(['--spider', self.spider_name,
                                            '-m', raw_json_string,
                                            '-c', 'parse_request_with_meta',
+                                           '--verbose',
                                            self.url('/html')])
         self.assertIn("DEBUG: It Works!", _textmode(stderr))
 
@@ -132,6 +135,7 @@ ITEM_PIPELINES = {'%s.pipelines.MyPipeline': 1}
         _, _, stderr = yield self.execute(['--spider', self.spider_name,
                                            '--cbkwargs', raw_json_string,
                                            '-c', 'parse_request_with_cb_kwargs',
+                                           '--verbose',
                                            self.url('/html')])
         self.assertIn("DEBUG: It Works!", _textmode(stderr))
 
@@ -139,6 +143,7 @@ ITEM_PIPELINES = {'%s.pipelines.MyPipeline': 1}
     def test_request_without_meta(self):
         _, _, stderr = yield self.execute(['--spider', self.spider_name,
                                           '-c', 'parse_request_without_meta',
+                                          '--nolinks',
                                            self.url('/html')])
         self.assertIn("DEBUG: It Works!", _textmode(stderr))
 
@@ -148,6 +153,7 @@ ITEM_PIPELINES = {'%s.pipelines.MyPipeline': 1}
         _, _, stderr = yield self.execute(['--spider', self.spider_name,
                                            '--pipelines',
                                            '-c', 'parse',
+                                           '--verbose',
                                            self.url('/html')])
         self.assertIn("INFO: It Works!", _textmode(stderr))
 
diff --git a/tests/test_item.py b/tests/test_item.py
index 010d3b141..947566686 100644
--- a/tests/test_item.py
+++ b/tests/test_item.py
@@ -1,9 +1,11 @@
 import sys
 import unittest
+from warnings import catch_warnings
 
 import six
 
-from scrapy.item import ABCMeta, Item, ItemMeta, Field
+from scrapy.exceptions import ScrapyDeprecationWarning
+from scrapy.item import ABCMeta, DictItem, Field, Item, ItemMeta
 from tests import mock
 
 
@@ -257,6 +259,17 @@ class ItemTest(unittest.TestCase):
         item['tags'].append('tag2')
         assert item['tags'] != copied_item['tags']
 
+    def test_dictitem_deprecation_warning(self):
+        """Make sure the DictItem deprecation warning is not issued for
+        Item"""
+        with catch_warnings(record=True) as warnings:
+            item = Item()
+            self.assertEqual(len(warnings), 0)
+            class SubclassedItem(Item):
+                pass
+            subclassed_item = SubclassedItem()
+            self.assertEqual(len(warnings), 0)
+
 
 class ItemMetaTest(unittest.TestCase):
 
@@ -302,5 +315,20 @@ class ItemMetaClassCellRegression(unittest.TestCase):
                 super(MyItem, self).__init__(*args, **kwargs)
 
 
+class DictItemTest(unittest.TestCase):
+
+    def test_deprecation_warning(self):
+        with catch_warnings(record=True) as warnings:
+            dict_item = DictItem()
+            self.assertEqual(len(warnings), 1)
+            self.assertEqual(warnings[0].category, ScrapyDeprecationWarning)
+        with catch_warnings(record=True) as warnings:
+            class SubclassedDictItem(DictItem):
+                pass
+            subclassed_dict_item = SubclassedDictItem()
+            self.assertEqual(len(warnings), 1)
+            self.assertEqual(warnings[0].category, ScrapyDeprecationWarning)
+
+
 if __name__ == "__main__":
     unittest.main()