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

Add start and stop parameters to the array.index() #76137

Closed
nikispahiev mannequin opened this issue Nov 6, 2017 · 14 comments
Closed

Add start and stop parameters to the array.index() #76137

nikispahiev mannequin opened this issue Nov 6, 2017 · 14 comments
Labels
3.10 extension-modules type-feature

Comments

@nikispahiev
Copy link
Mannequin

@nikispahiev nikispahiev mannequin commented Nov 6, 2017

BPO 31956
Nosy @rhettinger, @serhiy-storchaka, @Phaqui, @csabella, @corona10, @ZackerySpytz, @nanjekyejoannah, @kamilturek
PRs
  • #4435
  • #25059
  • Files
  • WIP-issue-31956
  • 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 2021-04-09.01:26:32.579>
    created_at = <Date 2017-11-06.11:13:47.630>
    labels = ['extension-modules', 'type-feature', '3.10']
    title = 'Add start and stop parameters to the array.index()'
    updated_at = <Date 2021-04-09.01:26:32.577>
    user = 'https://bugs.python.org/nikispahiev'

    bugs.python.org fields:

    activity = <Date 2021-04-09.01:26:32.577>
    actor = 'corona10'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-04-09.01:26:32.579>
    closer = 'corona10'
    components = ['Extension Modules']
    creation = <Date 2017-11-06.11:13:47.630>
    creator = 'niki.spahiev'
    dependencies = []
    files = ['47261']
    hgrepos = []
    issue_num = 31956
    keywords = ['patch']
    message_count = 14.0
    messages = ['305629', '305643', '305650', '305758', '306133', '306143', '306150', '306178', '330746', '350405', '350619', '352499', '369732', '390070']
    nosy_count = 10.0
    nosy_names = ['rhettinger', 'serhiy.storchaka', 'Phaqui', 'cheryl.sabella', 'corona10', 'ZackerySpytz', 'niki.spahiev', 'nanjekyejoannah', 'Ryan G.', 'kamilturek']
    pr_nums = ['4435', '25059']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue31956'
    versions = ['Python 3.10']

    @nikispahiev
    Copy link
    Mannequin Author

    @nikispahiev nikispahiev mannequin commented Nov 6, 2017

    Sequence protocol specifies 2 optional argument for index method:
    seq.index(value, [start, [stop]])

    array.index(value) needs start and stop arguments too.

    @nikispahiev nikispahiev mannequin added stdlib 3.8 labels Nov 6, 2017
    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Nov 6, 2017

    +1

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Nov 6, 2017

    Николай Спахиев: Are you only asking for the feature, or are you interested to work on a concrete patch?

    @nikispahiev
    Copy link
    Mannequin Author

    @nikispahiev nikispahiev mannequin commented Nov 7, 2017

    I plan to work on a patch.

    @phaqui
    Copy link
    Mannequin

    @phaqui phaqui mannequin commented Nov 12, 2017

    I decided to work on this, and I would like some review, as this would be my second contribution to cpython. Also, a general question:

    As I defined the start and end arguments Py_ssize_t, bigger indexes (more negative or more positive) than what can fit in Py_ssize_t will trigger an overflow error. This should be OK, though, as other functions with index arguments has them as Py_ssize_t - and getarrayitem() itself accepts a Py_ssize_t. Or?

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Nov 13, 2017

    Just take list.index as an example.

    @serhiy-storchaka serhiy-storchaka added extension-modules 3.7 type-feature and removed stdlib 3.8 labels Nov 13, 2017
    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Nov 13, 2017

    Anders Lorentsen: Py_ssize_t is the correct type for an index in the C language. It's not technically possible to create an array object larger than PY_SSIZE_T_MAX items :-)

    Serhiy> Just take list.index as an example.

    I concur with Serhiy :-)

    @phaqui
    Copy link
    Mannequin

    @phaqui phaqui mannequin commented Nov 14, 2017

    Writing my tests, I originally looked at Lib/test/seq_tests.py. One test case uses indexes that are (+-)4*sys.maxsize. This does not fit in Py_ssize_t, and so these tests cause my array implementation to raise an overflow exception.

    A solution is of course to have the function take general objects instead, and then truncate them down to a number that can fit in Py_ssize_t if it's too negative or positive).

    But I concur. It seems more reasonable to stay consistent with the rest of the module, too.

    I'll look over the test code to make sure I test for every given scenario (or as many as I can think of), and prepare a PR for this, then :)

    @RyanG
    Copy link
    Mannequin

    @RyanG RyanG mannequin commented Nov 30, 2018

    This functionality is useful to me. Is this issue still alive? If not, how can I help?

    @nanjekyejoannah
    Copy link
    Contributor

    @nanjekyejoannah nanjekyejoannah commented Aug 24, 2019

    There is a pending PR here : #4435 by phaqui. @Phaqui do you want to finish your PR ?

    @phaqui
    Copy link
    Mannequin

    @phaqui phaqui mannequin commented Aug 27, 2019

    As far as I can recall, the patch is generally speaking good to go. A number of discussions arose on various details, however. In any event, I'll take a look at it during the next few days.

    @phaqui
    Copy link
    Mannequin

    @phaqui phaqui mannequin commented Sep 15, 2019

    I have actually managed to lost my local branch of this fix, though I assume I can just start another one, manually copy over the changes, somehow mark this current PR as cancelled, aborted, or in my option the best: "replaced/superseeded by: [new PR]". In any case, there were discussions that seem to be unresolved, allow me to summarize:

    • Document that index() raises ValueError when *value* is not found?

    vstinner: We don't do this, remove this addition.
    serhiy: Other index() methods does this.
    ---> My patch current does this. Who has final saying here?

    • 'start' and 'stop' arguments are not keyword arguments, and also not shown in the signature as '.. start=0 ..' for this very reason (it may make them look as keyword arguments). Also, this lines up with list.index() for consistency. Wishes about changing this for all index()-methods has been expressed, but it seems to be consensus on doing this in unison for all index()-methods at once, in a bigger change... So, what is currently in the PR is good enough for now, or?

    • Wording in documentation: Clarify that "the returned index is still relative to the start of the array, not the searched sub sequence" or not?

    • Comment in the code about checking the length of the array on each iteration? There were comments about it being "confusing" - and while I agree, the other index()-code for lists, does not comment on this. Again I followed the path of most consistency, but I did receive comments about this. Yes to descriptive comments, or not?

    ----

    Generally speaking: In the end, all I really did was mimic how list.index() is both written and documented, and that's when discussions about issues related to that started occurring, and so I now remember that I halted my PR, waiting for these issues to be resolved.

    @csabella
    Copy link
    Contributor

    @csabella csabella commented May 23, 2020

    I found this SO about reclaiming an orphaned pull request:
    https://stackoverflow.com/a/53383772

    But you may still need to open a new PR for it.

    @corona10
    Copy link
    Member

    @corona10 corona10 commented Apr 2, 2021

    New changeset afd1265 by Zackery Spytz in branch 'master':
    bpo-31956: Add start and stop parameters to array.index() (GH-25059)
    afd1265

    @corona10 corona10 added 3.10 and removed 3.7 labels Apr 9, 2021
    @corona10 corona10 closed this as completed Apr 9, 2021
    @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
    3.10 extension-modules type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants