Skip to content
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

range purports to implement the Sequence ABC, but is missing index and count methods #53459

Closed
stutzbach mannequin opened this issue Jul 9, 2010 · 13 comments
Closed
Labels
interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@stutzbach
Copy link
Mannequin

stutzbach mannequin commented Jul 9, 2010

BPO 9213
Nosy @birkenfeld, @rhettinger, @mdickinson, @merwok, @durban
Files
  • issue9213.diff: Patch (py3k branch)
  • issue9213a.diff: Patch (fixed refleak + tests) (py3k branch)
  • issue9213b.diff: Adds support for non-longs, drops support for start/stop
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2010-09-14.16:54:05.428>
    created_at = <Date 2010-07-09.19:06:14.231>
    labels = ['interpreter-core', 'type-bug']
    title = 'range purports to implement the Sequence ABC, but is missing index and count methods'
    updated_at = <Date 2010-09-14.16:54:05.426>
    user = 'https://bugs.python.org/stutzbach'

    bugs.python.org fields:

    activity = <Date 2010-09-14.16:54:05.426>
    actor = 'stutzbach'
    assignee = 'stutzbach'
    closed = True
    closed_date = <Date 2010-09-14.16:54:05.428>
    closer = 'stutzbach'
    components = ['Interpreter Core']
    creation = <Date 2010-07-09.19:06:14.231>
    creator = 'stutzbach'
    dependencies = []
    files = ['18111', '18128', '18714']
    hgrepos = []
    issue_num = 9213
    keywords = ['patch']
    message_count = 13.0
    messages = ['109784', '109785', '111112', '111195', '114534', '114633', '115396', '116018', '116120', '116146', '116352', '116372', '116410']
    nosy_count = 6.0
    nosy_names = ['georg.brandl', 'rhettinger', 'mark.dickinson', 'stutzbach', 'eric.araujo', 'daniel.urban']
    pr_nums = []
    priority = 'critical'
    resolution = 'accepted'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue9213'
    versions = ['Python 3.2']

    @stutzbach
    Copy link
    Mannequin Author

    stutzbach mannequin commented Jul 9, 2010

    >>> isinstance(range, collections.Sequence)
    True
    >>> [method for method in set(dir(collections.Sequence)) - set(dir(range(1))) if not method.startswith('_')]
    ['index', 'count']

    @stutzbach stutzbach mannequin self-assigned this Jul 9, 2010
    @stutzbach stutzbach mannequin added interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Jul 9, 2010
    @stutzbach
    Copy link
    Mannequin Author

    stutzbach mannequin commented Jul 9, 2010

    In Python 2.6 and 2.7, the same problem applies to xrange objects.

    @durban
    Copy link
    Mannequin

    durban mannequin commented Jul 21, 2010

    The attached patch adds the range.count and range.index methods.
    Pseudocode for the two method:

    def count(self, ob):
        if ob in self:
            return 1
        else:
            return 0
    
    def index(self, ob, start=0, stop=len(self)):
        if ob in self:
            idx = (ob - self.start) // self.step
            if start < 0:
                start = start + len(self)
            if stop < 0:
                stop = stop + len(self)
            if start <= idx < stop:
                return idx
        raise ValueError("{!r} is not in range".format(ob))

    @durban
    Copy link
    Mannequin

    durban mannequin commented Jul 22, 2010

    Sorry, the previous patch has a reference leak. I'm attaching the fixed patch as issue9213a.diff (I also added a few tests with really big ranges).

    @merwok
    Copy link
    Member

    merwok commented Aug 21, 2010

    I can’t comment on C code, but the tests look good to me.

    @rhettinger
    Copy link
    Contributor

    rhettinger commented Aug 22, 2010

    I support adding index() and count() to range objects.

    @stutzbach
    Copy link
    Mannequin Author

    stutzbach mannequin commented Sep 2, 2010

    Thank you for the patch.

    The patch doesn't handle the case where the object being searched for is !PyLong_CheckExact. For example, the user might pass in a sub-type of int. The existing range_contains supports that case, so it seems like we should support it in range_index and range_count.

    The Sequence ABC's .index method doesn't include the optional start and stop parameters that are present on list.index. Since it's not part of the ABC, it's not mandatory that we implement those for range.index. The patch includes support for start and stop.

    Attached is a greatly revised patch, with two significant changes:

    • Adds support for !PyLong_CheckExact (using _PySequence_IterSearch)
    • Drops support for the start and stop parameters to index

    Dropping support for the start and stop parameters greatly simplified the code. If we want to support start and stop, then the code will need to get more complicated to handle the !PyLong_CheckExact case (since PySequence_IterSearch doesn't support start and stop).

    My patch abstracts out most of the code that had originally been in range_contains into a new function range_contains_long so that it can be called by range_contains, range_count, and range_index. The diff makes that part look like a large change, but it's mostly a whitespace change (the refactored code lost one indentation level but is otherwise unchanged).

    I uploaded Daniel Urban's patch and mine to Rietveld:
    http://codereview.appspot.com/2146041/

    Any strong feelings on whether range.index should support the start and stop arguments provided by list.index, tuple.index, and str.index, but not by collections.Sequence.index?

    @durban
    Copy link
    Mannequin

    durban mannequin commented Sep 10, 2010

    Attached is a greatly revised patch, with two significant changes:

    • Adds support for !PyLong_CheckExact (using _PySequence_IterSearch)

    Thanks, indeed, we should support that.

    Any strong feelings on whether range.index should support the start and
    stop arguments provided by list.index, tuple.index, and str.index, but
    not by collections.Sequence.index?

    I don't think that this is very important.

    @birkenfeld
    Copy link
    Member

    birkenfeld commented Sep 11, 2010

    I'd like to have this in 3.2.

    @stutzbach
    Copy link
    Mannequin Author

    stutzbach mannequin commented Sep 12, 2010

    Thanks, Georg, for prodding. As a new committer, I have possibly been erring a little too far on the side of having my patches thoroughly reviewed before committing.

    I'll commit the patch on Monday if no one raises objections (after re-testing, of course).

    @stutzbach
    Copy link
    Mannequin Author

    stutzbach mannequin commented Sep 13, 2010

    Committed as r84791.

    Question: should this bugfix be backported to Python 3.1 and to xrange objects in Python 2.7? Since it's a bugfix that adds new methods, it's a gray-area. (same question applies to the closely related Issue bpo-9212)

    @birkenfeld
    Copy link
    Member

    birkenfeld commented Sep 14, 2010

    I am -1 on adding new methods to builtins in bugfix releases.

    @stutzbach
    Copy link
    Mannequin Author

    stutzbach mannequin commented Sep 14, 2010

    Sounds reasonable to me. I'll close this and the related 9212 (both fixes are already committed to the py3k branch).

    @stutzbach stutzbach mannequin closed this as completed Sep 14, 2010
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants