-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-120802: Providing more information on __getitem__.
#122178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2859,33 +2859,83 @@ through the object's keys; for sequences, it should iterate through the values. | |||||||||||||||
|
|
||||||||||||||||
| .. note:: | ||||||||||||||||
|
|
||||||||||||||||
| Slicing is done exclusively with the following three methods. A call like :: | ||||||||||||||||
| Slicing is done exclusively with the following three methods. | ||||||||||||||||
| :meth:`__getitem__`, :meth:`__setitem__`, :meth:`__delitem__`. | ||||||||||||||||
| Expressions like: :: | ||||||||||||||||
|
|
||||||||||||||||
| a[1:2] = b | ||||||||||||||||
| a[1:2] | ||||||||||||||||
| a[1:2,] | ||||||||||||||||
| a[1:2, 2:3] | ||||||||||||||||
|
|
||||||||||||||||
| is translated to :: | ||||||||||||||||
| are translated into:. :: | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| a[slice(1, 2, None)] = b | ||||||||||||||||
| a[slice(1, 2, None)] | ||||||||||||||||
| a[(slice(1, 2, None),)] | ||||||||||||||||
| a[(slice(1, 2, None), slice(2, 3, None))] | ||||||||||||||||
|
|
||||||||||||||||
| and so forth. Missing slice items are always filled in with ``None``. | ||||||||||||||||
| Missing slice items are always filled in with ``None``. | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| .. method:: object.__getitem__(self, key) | ||||||||||||||||
|
|
||||||||||||||||
| Called to implement evaluation of ``self[key]``. For :term:`sequence` types, | ||||||||||||||||
| the accepted keys should be integers. Optionally, they may support | ||||||||||||||||
| :class:`slice` objects as well. Negative index support is also optional. | ||||||||||||||||
| If *key* is | ||||||||||||||||
| of an inappropriate type, :exc:`TypeError` may be raised; if *key* is a value | ||||||||||||||||
| outside the set of indexes for the sequence (after any special | ||||||||||||||||
| interpretation of negative values), :exc:`IndexError` should be raised. For | ||||||||||||||||
| :term:`mapping` types, if *key* is missing (not in the container), | ||||||||||||||||
| :exc:`KeyError` should be raised. | ||||||||||||||||
| Called to implement evaluation of ``self[key]``. There are 3 supported | ||||||||||||||||
| duck types: :term:`sequence` types, :term:`mapping` types, and any type | ||||||||||||||||
| with custom behaviour on indexing. Syntax doesn't perform any checks, | ||||||||||||||||
| so it is this method responsibility to raise :exc:`TypeError` on keys of | ||||||||||||||||
| improper type. | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Comment on lines
+2883
to
+2885
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| First, there is expected behaviour for :term:`sequence` types:. | ||||||||||||||||
| Minimal requirement for :func:`iter` to work (without existing | ||||||||||||||||
| :meth:`__iter__` method) is that, ``__getitem__`` needs to accept | ||||||||||||||||
| integers from 0 up to the index, on which :exc:`IndexError` is raised, | ||||||||||||||||
| stoping the iteration. :func:`reversed` will also call ``len(self)`` and | ||||||||||||||||
| request only items at non-negative keys. However, if class passes as | ||||||||||||||||
| :class:`collections.abc.Sequence`, ``__getitem__`` should also:. | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| 1. Accept negative integer keys, for indexing from the end of the sequence. | ||||||||||||||||
| 2. For all integer keys outside of bounds ``-len(self) <= key < len(self)``, | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should not necessarily be the case. For instance, if you implement a list that reduces the index modulo its length. |
||||||||||||||||
| raise :exc:`TypeError`. | ||||||||||||||||
| 3. For non-integer key that supports :func:`operator.index`, use that | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Swap the wording (put "Use..." at the front). |
||||||||||||||||
| index as integer key. | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should probably raise a TypeError otherwise. |
||||||||||||||||
| 4. Fully support slicing. If possible, it should return the same type, | ||||||||||||||||
| if not, it should return a similar type. Meaning, ``self[::]`` should | ||||||||||||||||
| create shallow copy. | ||||||||||||||||
|
|
||||||||||||||||
| Second, there is expected behaviour for :term:`mapping` types:. | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
| ``__getitem__`` should return element for given *key*, if *key* is missing | ||||||||||||||||
| (not in the container), :exc:`KeyError` should be raised. | ||||||||||||||||
| To avoid silent errors, ``__getitem__`` should only implement mapping | ||||||||||||||||
| (or not implement it at all), but if needed, it could be extended | ||||||||||||||||
| for *keys* of different type. For implicit (ABC) children of | ||||||||||||||||
| :class:`collections.abc.Mapping`, static type checkers are allowed to | ||||||||||||||||
| deduce *keys* type as all types accepted by ``__getitem__``. | ||||||||||||||||
| To avoid such errors, :term:`mapping` types with extended ``__getitem__`` | ||||||||||||||||
| need to override this behaviour by explicitly deriving from | ||||||||||||||||
| :ref:`types-genericalias` of :class:`collections.abc.Mapping`. | ||||||||||||||||
|
Comment on lines
+2909
to
+2914
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This paragraph is too compact for the reader. You should split the requirements from the static type checkers (which I'm not sure should be mentioned here at all). |
||||||||||||||||
|
|
||||||||||||||||
| Lastly, here is an example of possible implementation for ``Matrix``, | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When you say "first, second, lastly", it appears like you are describing requirements but all of them are actually unrelated to each other. You can just start your paragraphs normally like "For Sequence types, ...", "For mapping types,..." and use an Example section. |
||||||||||||||||
| that is also a ``Sequence[Sequence]``:. :: | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
General tips: Don't use a period after a colon and use |
||||||||||||||||
|
|
||||||||||||||||
| def __getitem__(self, key): | ||||||||||||||||
| if getattr(key, '__index__', None) is not None: | ||||||||||||||||
| return self._rows_list[key] | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would be |
||||||||||||||||
| if isinstance(key, slice): | ||||||||||||||||
| return Matrix(self._rows_list[key]) | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You return is not covariant for subclasses here. Ideally, it should be |
||||||||||||||||
| try: | ||||||||||||||||
| [key1, key2] = key | ||||||||||||||||
| except ValueError as error: | ||||||||||||||||
| raise TypeError from error | ||||||||||||||||
| if isinstance(key1, slice): | ||||||||||||||||
| indices = range(*key1.indices(len(self._rows_list))) | ||||||||||||||||
| if isinstance(key2, slice): | ||||||||||||||||
| return Matrix([self._rows_list[i][key2] for i in indices]) | ||||||||||||||||
| return [self._rows_list[i][key2] for i in indices] #sliced column | ||||||||||||||||
| return self._rows_list[key1][key2] #element or sliced row | ||||||||||||||||
|
|
||||||||||||||||
| .. note:: | ||||||||||||||||
|
|
||||||||||||||||
| :keyword:`for` loops expect that an :exc:`IndexError` will be raised for | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where did this note disappear? |
||||||||||||||||
| illegal indexes to allow proper detection of the end of the sequence. | ||||||||||||||||
| It is possible to have an object, that is both :term:`sequence` and | ||||||||||||||||
| :term:`mapping`. | ||||||||||||||||
|
|
||||||||||||||||
| .. note:: | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -2915,8 +2965,8 @@ through the object's keys; for sequences, it should iterate through the values. | |||||||||||||||
|
|
||||||||||||||||
| .. method:: object.__missing__(self, key) | ||||||||||||||||
|
|
||||||||||||||||
| Called by :class:`dict`\ .\ :meth:`__getitem__` to implement ``self[key]`` for dict subclasses | ||||||||||||||||
| when key is not in the dictionary. | ||||||||||||||||
| Called by :class:`dict`\ .\ :meth:`__getitem__` to implement ``self[key]``, | ||||||||||||||||
| for dict subclasses, when key is not in the dictionary. | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| .. method:: object.__iter__(self) | ||||||||||||||||
|
|
@@ -2926,18 +2976,33 @@ through the object's keys; for sequences, it should iterate through the values. | |||||||||||||||
| objects in the container. For mappings, it should iterate over the keys of | ||||||||||||||||
| the container. | ||||||||||||||||
|
|
||||||||||||||||
| If the :meth:`__iter__` method is missing, meaning the attribute is missing | ||||||||||||||||
| (set to :const:`None` is not considered missing), the :func:`iter` built-in | ||||||||||||||||
|
Comment on lines
+2979
to
+2980
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well... I would say that it is obvious that setting it to |
||||||||||||||||
| will fall back to using the sequence :meth:`__getitem__`. Objects that support | ||||||||||||||||
| the sequence protocol should only provide :meth:`__reversed__`, if they can | ||||||||||||||||
| provide an implementation, that is more efficient than the one provided by | ||||||||||||||||
| :func:`reversed`. If not, they should copy the implementation from ABC-s | ||||||||||||||||
| (to support them), by being a child, or by explicit assignment:. :: | ||||||||||||||||
|
|
||||||||||||||||
| __iter__ = collections.abc.Sequence.__iter__ | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| .. method:: object.__reversed__(self) | ||||||||||||||||
|
|
||||||||||||||||
| Called (if present) by the :func:`reversed` built-in to implement | ||||||||||||||||
| reverse iteration. It should return a new iterator object that iterates | ||||||||||||||||
| over all the objects in the container in reverse order. | ||||||||||||||||
|
|
||||||||||||||||
| If the :meth:`__reversed__` method is not provided, the :func:`reversed` | ||||||||||||||||
| If the :meth:`__reversed__` method is missing, meaning the attribute is missing | ||||||||||||||||
| (set to :const:`None` is not considered missing), the :func:`reversed` | ||||||||||||||||
|
Comment on lines
+2996
to
+2997
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is too verbose. |
||||||||||||||||
| built-in will fall back to using the sequence protocol (:meth:`__len__` and | ||||||||||||||||
| :meth:`__getitem__`). Objects that support the sequence protocol should | ||||||||||||||||
| only provide :meth:`__reversed__` if they can provide an implementation | ||||||||||||||||
| only provide :meth:`__reversed__`, if they can provide an implementation, | ||||||||||||||||
| that is more efficient than the one provided by :func:`reversed`. | ||||||||||||||||
| If not, they should copy the implementation from ABC-s (to support them), | ||||||||||||||||
| by being a child, or by explicit assignment:. :: | ||||||||||||||||
|
|
||||||||||||||||
| __reversed__ = collections.abc.Sequence.__reversed__ | ||||||||||||||||
|
Comment on lines
+3002
to
+3005
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this should be given. |
||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| The membership test operators (:keyword:`in` and :keyword:`not in`) are normally | ||||||||||||||||
|
|
||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.