Skip to content

bpo-47178: Improve the implementations of Sequence.index and MutableSequence.extend in collections.abc#32150

Closed
geryogam wants to merge 8 commits intopython:mainfrom
geryogam:patch-32
Closed

bpo-47178: Improve the implementations of Sequence.index and MutableSequence.extend in collections.abc#32150
geryogam wants to merge 8 commits intopython:mainfrom
geryogam:patch-32

Conversation

@geryogam
Copy link
Copy Markdown
Contributor

@geryogam geryogam commented Mar 28, 2022

This P.R. will make the following changes to the collections.abc module:

  • simplify the implementation with slicing in function Sequence.index.
  • remove an unnecessary copy of self when a sequence extends itself in function MutableSequence.extend.

https://bugs.python.org/issue47178

Comment thread Lib/_collections_abc.py Outdated
Comment thread Lib/_collections_abc.py Outdated
@geryogam geryogam changed the title Move additional code out of a try clause in function Sequence.index Improve the implementations of Sequence.index and MutableSequence.extend in collections.abc Mar 30, 2022
@geryogam geryogam marked this pull request as ready for review March 30, 2022 16:56
@geryogam geryogam requested a review from rhettinger as a code owner March 30, 2022 16:56
@geryogam geryogam changed the title Improve the implementations of Sequence.index and MutableSequence.extend in collections.abc bpo-47178: Improve the implementations of Sequence.index and MutableSequence.extend in collections.abc Mar 31, 2022
Comment thread Lib/_collections_abc.py
@bedevere-bot
Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Comment thread Lib/_collections_abc.py
Comment thread Lib/_collections_abc.py
@rhettinger
Copy link
Copy Markdown
Contributor

Am marking this as closed because the changes are incorrect.

@rhettinger rhettinger closed this Mar 31, 2022
@rhettinger
Copy link
Copy Markdown
Contributor

Example that succeeds with the current code but fails with the proposed revision to index()

class LL(Sequence):
    def __len__(self):
        return 3
    def __getitem__(self, i):
        if i >= 10:
            raise IndexError
        return i * 10

assert LL().index(70) == 7

Here the issue is that when stop is None, the index() code should iterate until it hits an IndexError rather than relying on _len_ which can be inaccurate or can reflect an actual size change during iteration.

@geryogam
Copy link
Copy Markdown
Contributor Author

geryogam commented Apr 5, 2022

Thanks for the example. Indeed, I overlooked that Sequence.index did not rely on Sequence.__len__ but on IndexError to stop, except for shifting negative start and stop indices. My PR was incorrect, sorry.

I have opened a separate PR that moves the unrelated if statement out of the try clause.

@geryogam geryogam deleted the patch-32 branch April 5, 2022 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants