Skip to content

bpo-31956: Add start and stop parameters to array.index() #4435

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.

Sign up for GitHub

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
Phaqui wants to merge 9 commits into from
Closed

bpo-31956: Add start and stop parameters to array.index() #4435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
55d18ca
Implementation and first test cases
Phaqui Nov 17, 2017
f9ddd58
Updated documentation for array.index()
Phaqui Nov 17, 2017
94118b2
Added news entry in NEWS.d/next/
Phaqui Nov 17, 2017
34692ba
Clean up according to PR review
Phaqui Nov 17, 2017
a34e960
Simplified documentation
Phaqui Nov 17, 2017
3ca98de
Fix crash when an object mutates the array in its __eq__ method
Phaqui Nov 17, 2017
e6d75ae
*start* and *stop*
Phaqui Nov 17, 2017
d2b2f9f
Simplified news blurb. Having start and stop arguments means they aut…
Phaqui Nov 17, 2017
d2755af
Merge branch 'master' into fix-issue-31956
serhiy-storchaka Feb 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions Doc/library/array.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,11 +188,16 @@ The following data items and methods are also supported:
array of some other type.


.. method:: array.index(x)
.. method:: array.index(x[, start[, stop]])

@vstinner vstinner Nov 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can document the default start value:

array.index(x, start=0[, stop])

@serhiy-storchaka serhiy-storchaka Nov 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would prefer to specify start=0, stop=sys.maxsize, but there were objections against this, because this looks like start and stop are keyword parameters. There were issues about changing the documentation back to [, start[, stop]].


Return the smallest *i* such that *i* is the index of the first occurrence of
*x* in the array.
*x* in the array. The optional arguments *start* and *stop* can be specified to
search for *x* within a subsection of the array.

Raises :exc:`ValueError` if *x* is not found.

.. versionchanged:: 3.7
Added optional *start* and *stop* parameters.

.. method:: array.insert(i, x)

Expand Down
11 changes: 11 additions & 0 deletions Lib/test/test_array.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -871,6 +871,17 @@ def test_index(self):
self.assertRaises(ValueError, a.index, None)
self.assertRaises(ValueError, a.index, self.outside)

a = array.array('i', [-2, -1, 0, 0, 1, 2])
self.assertEqual(a.index(0), 2)
self.assertEqual(a.index(0, 2), 2)
self.assertEqual(a.index(0, -4), 2)
self.assertEqual(a.index(-2, -10), 0)
self.assertEqual(a.index(0, 3), 3)
self.assertEqual(a.index(0, -3), 3)
self.assertEqual(a.index(0, 3, 4), 3)
self.assertEqual(a.index(0, -3, -2), 3)
self.assertRaises(ValueError, a.index, 2, 0, -10)

def test_count(self):
example = 2*self.example
a = array.array(self.typecode, example)
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2017-11-17-06-18-05.bpo-31956.m3Ylau.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
array.index() now supports optional start and stop arguments.
19 changes: 16 additions & 3 deletions Modules/arraymodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1115,18 +1115,31 @@ array_array_count(arrayobject *self, PyObject *v)
array.array.index

v: object
start: slice_index(accept={int}) = 0
stop: slice_index(accept={int}, c_default="PY_SSIZE_T_MAX") = sys.maxsize

@vstinner vstinner Nov 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it possible to not document the default value at Python level? Try to remove "= sys.maxsize".

Maybe you should put "/" (currently the following line) between start and stop?

@serhiy-storchaka serhiy-storchaka Nov 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be better to not make stop the keyword argument until other index() methods accept keyword arguments.

The signature of array.index() matches the signature of list.index() and this is the best that we can to do now. Due to limitations of pydoc, inspect and Argument Clinic the default value for stop is displayed as a concrete number, but this is a separate issue.

@vstinner vstinner Nov 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, right, I didn't notice that other methods don't accept keywords.

>>> "abc".index("c", stop=3)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: index() takes no keyword arguments

I like the idea of accepting keyword arguments in other index() methods :-)

@serhiy-storchaka serhiy-storchaka Nov 18, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think there is an open issue for this. But first I want unify the code for all index() methods. Currently index() methods of str/bytes/bytearray use slightly different code.

/

Return index of first occurrence of v in the array.

Raises ValueError if the value is not present.

@vstinner vstinner Nov 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Usually we don't document the exact exception raised by a method. Please remove this addition.

Maybe we should start to document them, but I would prefer to have a discussion about that, before starting to document them.

@serhiy-storchaka serhiy-storchaka Nov 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Docstrings of other index() methods document this.

[clinic start generated code]*/

static PyObject *
array_array_index(arrayobject *self, PyObject *v)
/*[clinic end generated code: output=d48498d325602167 input=cf619898c6649d08]*/
array_array_index_impl(arrayobject *self, PyObject *v, Py_ssize_t start,
Py_ssize_t stop)
/*[clinic end generated code: output=c45e777880c99f52 input=6efdb3dae558556c]*/
{
Py_ssize_t i;

@serhiy-storchaka serhiy-storchaka Nov 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove redundant empty lines.

for (i = 0; i < Py_SIZE(self); i++) {
if (start < 0) {
start += Py_SIZE(self);
if (start < 0)
start = 0;
}
if (stop < 0) {
stop += Py_SIZE(self);

@vstinner vstinner Nov 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is stop is still negative here?

Maybe you can use PySlice_AdjustIndices() here with step=1? What do you think @serhiy-storchaka?

@serhiy-storchaka serhiy-storchaka Nov 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All correct if stop is still negative. The following loop just will not be executed.

}
for (i = start; i < stop && i < Py_SIZE(self); i++) {

@vstinner vstinner Nov 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess that you kept the "< Py_SIZE()" check if the array is mutated during the PyObject_RichCompareBool() call? If yes, please add a short comment to explain that, since it can be surprising.

@serhiy-storchaka serhiy-storchaka Nov 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggested to do this. This is an exact copy from list.index(). Do you want to add a comment in list.index()?

PyObject *selfi;
int cmp;

Expand Down
32 changes: 28 additions & 4 deletions Modules/clinic/arraymodule.c.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,37 @@ PyDoc_STRVAR(array_array_count__doc__,
{"count", (PyCFunction)array_array_count, METH_O, array_array_count__doc__},

PyDoc_STRVAR(array_array_index__doc__,
"index($self, v, /)\n"
"index($self, v, start=0, stop=sys.maxsize, /)\n"
"--\n"
"\n"
"Return index of first occurrence of v in the array.");
"Return index of first occurrence of v in the array.\n"
"\n"
"Raises ValueError if the value is not present.");

#define ARRAY_ARRAY_INDEX_METHODDEF \
{"index", (PyCFunction)array_array_index, METH_O, array_array_index__doc__},
{"index", (PyCFunction)array_array_index, METH_FASTCALL, array_array_index__doc__},

static PyObject *
array_array_index_impl(arrayobject *self, PyObject *v, Py_ssize_t start,
Py_ssize_t stop);

static PyObject *
array_array_index(arrayobject *self, PyObject *const *args, Py_ssize_t nargs)
{
PyObject *return_value = NULL;
PyObject *v;
Py_ssize_t start = 0;
Py_ssize_t stop = PY_SSIZE_T_MAX;

if (!_PyArg_ParseStack(args, nargs, "O|O&O&:index",
&v, _PyEval_SliceIndexNotNone, &start, _PyEval_SliceIndexNotNone, &stop)) {
goto exit;
}
return_value = array_array_index_impl(self, v, start, stop);

exit:
return return_value;
}

PyDoc_STRVAR(array_array_remove__doc__,
"remove($self, v, /)\n"
Expand Down Expand Up @@ -505,4 +529,4 @@ PyDoc_STRVAR(array_arrayiterator___setstate____doc__,

#define ARRAY_ARRAYITERATOR___SETSTATE___METHODDEF \
{"__setstate__", (PyCFunction)array_arrayiterator___setstate__, METH_O, array_arrayiterator___setstate____doc__},
/*[clinic end generated code: output=1289bde2a095a712 input=a9049054013a1b77]*/
/*[clinic end generated code: output=8823a7df5745072f input=a9049054013a1b77]*/