Skip to content

Add SimpleSequence as preferred alias for SupportsLenAndGetItem #6099

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

Closed
wants to merge 2 commits into from

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented Sep 30, 2021

This protocol represents a sequence as defined in the Python
documentation, as opposed to collections.abc.Sequence.

This protocol represents a sequence as defined in the Python
documentation, as opposed to collections.abc.Sequence.
@srittau
Copy link
Collaborator Author

srittau commented Sep 30, 2021

Maybe we can find a better name? But it should have Sequence in there.

@Akuli
Copy link
Collaborator

Akuli commented Sep 30, 2021

I think the name should be super obvious, so that you immediately know what it does when you see it somewhere in the code, such as SupportsLenAndGetItem :)

@srittau
Copy link
Collaborator Author

srittau commented Sep 30, 2021

There already is a concept in Python that expresses this, "sequence". We should use this name to make it clear that this protocol actually reflect that concept.

@Akuli
Copy link
Collaborator

Akuli commented Sep 30, 2021

What about functions that only require the sequence to have a __getitem__ method, such as iter()? There is no one single type that describes "what you actually want to use when it's tempting to use Sequence", as that may or may not require __len__ to exist, so I don't think we should make a protocol whose name suggest that it is "what you actually want to use when it's tempting to use Sequence".

@Akuli
Copy link
Collaborator

Akuli commented Sep 30, 2021

Actually nevermind. A type like "what docs describe as Sequence" is fine, because it is very uncommon to implement __getitem__ without __len__, so I don't think this will be a problem in practice. And in many cases, SupportsLenAndGetItem is an improvement over Sequence.

As a side note, I generally consider Sequence a bit of a code smell, because often the reality is "the code for loops over it" or "the code checks for lists and tuples with isinstance".

Copy link
Collaborator

@Akuli Akuli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intended to include this in my other review, but I think I clicked the wrong button.

Edit: Now my first review comment was lost somehow, I'll just write it here instead of trying to add another review and losing something again :) The new name should say "it is sequence the concept, not the class" or "it has __len__ and __getitem__". So something like SequenceAccordingToDocs, but that isn't a great name either.

def __len__(self) -> int: ...
def __getitem__(self, __k: int) -> _T_co: ...

# obsolete, use SimpleSequence instead
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just delete it? It was not marked with # stable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess if it's used in third party stubs (there seems to be the one single case), under some circumstances you could run into silent loss of type checking. This already ends up happening though, e.g. see #5786 and #5751, so I'd also be inclined to just delete it.

If we don't delete it, we should alias here, though, and still update the annoy stubs

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The annoy stubs were my main concern. If we update them, they won't work, until type checkers have been updated with the latest typeshed. I'd suggest keeping it for a few months.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly about this name vs SupportsLenAndGetItem. Typing users I interact with still get confused about just Sequence vs List :-)

@srittau
Copy link
Collaborator Author

srittau commented Sep 30, 2021

Edit: Now my first review comment was lost somehow, I'll just write it here instead of trying to add another review and losing something again :) The new name should say "it is sequence the concept, not the class" or "it has __len__ and __getitem__". So something like SequenceAccordingToDocs, but that isn't a great name either.

PythonSequence, GlossarySequence, NotThatOtherSequence, RealSequence

@Akuli
Copy link
Collaborator

Akuli commented Sep 30, 2021

As a maintainer, I would love it to be NotThatOtherSequence, but that isn't exactly very contributor friendly because most other people don't know the context.

@Akuli
Copy link
Collaborator

Akuli commented Oct 1, 2021

How about GetItemAndLenSequence? It seems to satisfy everyone's needs, as it very clearly says what an object must have to satisfy the protocol, and the name also hints that it is an alternative to the usual Sequence. I think it would be an improvement over SupportsLenAndGetItem, because there are mappings that also support __len__ and __getitem__, but the protocol isn't intended to be used with those.

@BvB93
Copy link
Contributor

BvB93 commented Oct 1, 2021

Yet another option would bo something like PySequence; this is the name used in the C-API to refer to this type of protocol.

@JelleZijlstra
Copy link
Member

I don't like PySequence; if a user sees a Sequence and a PySequence, how are they supposed to know which is which?

I'm OK with most other options presented here, but I prefer SimpleSequence for brevity or SupportsGetItemAndLen (the current version) for explicitness.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

bidict (https://github.com/jab/bidict)
- bidict/_base.py:395: error: Argument 1 to "reversed" has incompatible type "MutableMapping[KT, VT]"; expected "SupportsLenAndGetItem[KT]"  [arg-type]
+ bidict/_base.py:395: error: Argument 1 to "reversed" has incompatible type "MutableMapping[KT, VT]"; expected "SimpleSequence[KT]"  [arg-type]

@hauntsaninja
Copy link
Collaborator

I don't think we have consensus here, so closing as per #6563 (comment)

@srittau srittau deleted the plain-sequence branch May 21, 2022 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants