Repository: scrapy/itemadapter

Issue:
Should field_names preserve key views?
Currently .field_names method returns a list always. This is good from a consistency point of view, but it changes the algorithmic complexity of lookup from O(1) to O(N). 

I recall we discussed it in past, but maybe there is some solution possible which keeps the API consistent, but doesn't change lookups to O(N).

This is minor, as field name lists should be tiny in a real world.

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/scrapy/itemadapter/pull/18?src=pr&el=h1) Report
> Merging [#18](https://codecov.io/gh/scrapy/itemadapter/pull/18?src=pr&el=desc) into [master](https://codecov.io/gh/scrapy/itemadapter/commit/b9ab8eaf66928cd650466280e718b883a0c59987&el=desc) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/scrapy/itemadapter/pull/18/graphs/tree.svg?width=650&height=150&src=pr&token=MDGbx42SRs)](https://codecov.io/gh/scrapy/itemadapter/pull/18?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##            master       #18   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           93        93           
=========================================
  Hits            93        93           
```


| [Impacted Files](https://codecov.io/gh/scrapy/itemadapter/pull/18?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [itemadapter/adapter.py](https://codecov.io/gh/scrapy/itemadapter/pull/18/diff?src=pr&el=tree#diff-aXRlbWFkYXB0ZXIvYWRhcHRlci5weQ==) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/scrapy/itemadapter/pull/18?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/scrapy/itemadapter/pull/18?src=pr&el=footer). Last update [b9ab8ea...63ff8fa](https://codecov.io/gh/scrapy/itemadapter/pull/18?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Gallaecio: If performance is a concern, and assuming there is not a more efficient way to create a `KeysView`, we could cache the result in `ItemAdapter._field_names`.
elacuesta: > we could cache the result in ItemAdapter._field_names

I _believe_ we were doing that initially, and we switched it off because of simplicity. I can't find the commit though.

Patch:
diff --git a/README.md b/README.md
index cf0c7c4..01d95de 100644
--- a/README.md
+++ b/README.md
@@ -122,9 +122,10 @@ for `scrapy.item.Item`s
 * [`attr.Attribute.metadata`](https://www.attrs.org/en/stable/examples.html#metadata)
   for `attrs`-based items
 
-`field_names() -> List[str]`
+`field_names() -> KeysView`
 
-Return a list with the names of all the defined fields for the item.
+Return a [keys view](https://docs.python.org/3/library/collections.abc.html#collections.abc.KeysView)
+with the names of all the defined fields for the item.
 
 ### `is_item` function
 
diff --git a/itemadapter/adapter.py b/itemadapter/adapter.py
index 5e83590..ab7d037 100644
--- a/itemadapter/adapter.py
+++ b/itemadapter/adapter.py
@@ -1,6 +1,6 @@
-from collections.abc import MutableMapping
+from collections.abc import KeysView, MutableMapping
 from types import MappingProxyType
-from typing import Any, Iterator, List
+from typing import Any, Iterator, Optional
 
 from .utils import is_item, is_attrs_instance, is_dataclass_instance, is_scrapy_item
 
@@ -15,6 +15,7 @@ class ItemAdapter(MutableMapping):
         if not is_item(item):
             raise TypeError("Expected a valid item, got %r instead: %s" % (type(item), item))
         self.item = item
+        self._fields_dict = None  # type: Optional[dict]
 
     def __repr__(self) -> str:
         return "ItemAdapter for type %s: %r" % (self.item.__class__.__name__, self.item)
@@ -98,19 +99,25 @@ class ItemAdapter(MutableMapping):
         else:
             return MappingProxyType({})
 
-    def field_names(self) -> List[str]:
+    def field_names(self) -> KeysView:
         """
-        Return a list with the names of all the defined fields for the item
+        Return read-only key view with the names of all the defined fields for the item
         """
         if is_scrapy_item(self.item):
-            return list(self.item.fields.keys())
+            return KeysView(self.item.fields)
         elif is_dataclass_instance(self.item):
             import dataclasses
 
-            return [field.name for field in dataclasses.fields(self.item)]
+            if self._fields_dict is None:
+                self._fields_dict = {field.name: None for field in dataclasses.fields(self.item)}
+            return KeysView(self._fields_dict)
         elif is_attrs_instance(self.item):
             import attr
 
-            return [field.name for field in attr.fields(self.item.__class__)]
+            if self._fields_dict is None:
+                self._fields_dict = {
+                    field.name: None for field in attr.fields(self.item.__class__)
+                }
+            return KeysView(self._fields_dict)
         else:
-            return list(self.item.keys())
+            return KeysView(self.item)
diff --git a/tox.ini b/tox.ini
index 62f80b4..b806947 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,5 +1,5 @@
 [tox]
-envlist = py35,py36,py37,py38,flake8,typing,black
+envlist = flake8,typing,black,py35,py36,py37,py38
 
 [testenv]
 deps =


Test patch:
diff --git a/tests/test_adapter.py b/tests/test_adapter.py
index 5c84490..1bc7ac0 100644
--- a/tests/test_adapter.py
+++ b/tests/test_adapter.py
@@ -1,5 +1,6 @@
 import unittest
 from types import MappingProxyType
+from typing import KeysView
 
 from itemadapter.adapter import ItemAdapter
 
@@ -89,7 +90,7 @@ class BaseTestMixin:
     def test_field_names(self):
         item = self.item_class(name="asdf", value=1234)
         adapter = ItemAdapter(item)
-        self.assertIsInstance(adapter.field_names(), list)
+        self.assertIsInstance(adapter.field_names(), KeysView)
         self.assertEqual(sorted(adapter.field_names()), ["name", "value"])
 
 
@@ -149,6 +150,13 @@ class DictTestCase(unittest.TestCase, BaseTestMixin):
         for field_name in ("name", "value", "undefined_field"):
             self.assertEqual(adapter.get_field_meta(field_name), MappingProxyType({}))
 
+    def test_field_names_updated(self):
+        item = self.item_class(name="asdf")
+        field_names = ItemAdapter(item).field_names()
+        self.assertEqual(sorted(field_names), ["name"])
+        item["value"] = 1234
+        self.assertEqual(sorted(field_names), ["name", "value"])
+
 
 class ScrapySubclassedItemTestCase(NonDictTestMixin, unittest.TestCase):