gh-113773: add list.index() "key" named argument by kristjanvalur · Pull Request #113772 · python/cpython

kristjanvalur · 2024-01-06T14:18:43Z

This PR adds a keyword-only key argument to list.index().
When provided, each list item is transformed by the key callable before being compared to value, thus allowing
a functional-style search of an item in a list:

data = [(i, chr(i)) for i in range(100)]
index_of_a  = data.index("a", key=lambda i: i[1])

This is based on an idea by @rhettinger from the discussion in PR #112498, where a case for finding an object in a list via
keyed lookup is made. This PR applies that idea directly to the list object underlying the heap in question.

list.index() is not explicitly documented anywhere, so there is no doc update.

Issue: Add 'key' argument to list.index() #113773

grigoriev-semyon · 2024-01-06T15:15:27Z

+    if (!key)
+        for (i = start; i < stop && i < Py_SIZE(self); i++) {


you can add
int cmp before for if stmt

and modify for-loops like that:
for (i = start, cmp = 0; i < stop && i < Py_SIZE(self) && cmp == 0; i++)

reference:

cpython/Objects/listobject.c

Line 452 in d36a365

for (i = 0, cmp = 0 ; cmp == 0 && i < Py_SIZE(a); ++i) {

that's pretty pointless, also not the way the old code was. anyway, I've now modified it.

Eclips4 · 2024-01-06T15:33:20Z

-        else if (cmp < 0)
-            return NULL;
-    }
+    if (!key)


I was following the style of the cod,e but ok, pep 7 for new code.

Eclips4 · 2024-01-06T15:36:59Z

+            if (cmp > 0)
+                return PyLong_FromSsize_t(i);
+            else if (cmp < 0)
+                return NULL;


Suggested change

if (cmp > 0)

return PyLong_FromSsize_t(i);

else if (cmp < 0)

return NULL;

if (cmp < 0) {

return NULL;

}

return PyLong_FromSsize_t(i);

it is not eq when cmp == 0

Yep, you're right.
There's should be

if (cmp == -1) { return NULL; } else if (cmp == 1 ) { return PyLong_FromSsize_t(i); }

This is the original code, only indented. See newest version.

Eclips4 · 2024-01-06T15:39:40Z

+            if (cmp > 0)
+                return PyLong_FromSsize_t(i);
+            else if (cmp < 0)
+                return NULL;


Suggested change

if (cmp > 0)

return PyLong_FromSsize_t(i);

else if (cmp < 0)

return NULL;

if (cmp < 0) {

return NULL;

}

return PyLong_FromSsize_t(i);

#113772 (comment)

asgain, original code, with indent.

Eclips4 · 2024-01-06T15:39:53Z

+            else if (cmp < 0)
+                return NULL;
+        }
+    else


Eclips4 · 2024-01-06T15:40:08Z

+            Py_INCREF(item);
+            PyObject *obj = PyObject_CallFunctionObjArgs(key, item, NULL);
+            Py_DECREF(item);
+            if (!obj)


would be good if you pointed to the particular pep7 violation you have in mind. I don't think constructs such as !obj are forbidden, if that is what you mean.

Eclips4 · 2024-01-06T15:43:21Z

I do not know, would it be supported by others, but this kind of change definitely needs a NEWS entry and documentation update for list.index.

kristjanvalur · 2024-01-07T12:56:13Z

I do not know, would it be supported by others, but this kind of change definitely needs a NEWS entry and documentation update for list.index.

NEWS added. list.index is not documented anywhere except in-line.

sobolevn · 2024-01-07T19:38:25Z

    stop: slice_index(accept={int}, c_default="PY_SSIZE_T_MAX") = sys.maxsize
    /
+    *
+    key: object=NULL


AC has problems with NULL default. To check this please run inspect.signature(list.index) before and after.

are you saying that object=NULL is nowhere used in the code despite being a documented convenience way of signifying optional PyObject values? So, either fix AC, or the docs?
I was an active contributor before AC was introduced and I'm only learning it now.
I guess you want me to turn this into object=None and make comparisons with PyNone in the code.

There is a bunch of these in the code: https://github.com/search?q=repo%3Apython%2Fcpython+%22+object+%3D+null%22&type=code

sobolevn · 2024-01-07T19:38:51Z

+            del data[:]
+            return i[1]
+
+        with self.assertRaises(ValueError):


Please, assert the error message here. ValueError is too broad.

sobolevn · 2024-01-07T19:40:29Z

    if (start < 0) {
        start += Py_SIZE(self);
-        if (start < 0)
+        if (start < 0) {


I've seen other comments, but I have an oposite opinion: diff should be as minimal as possible. This is just a comment, no action needed, unless others also agree :)

I respectfully disagree, the style should be consistent within the function. Either I convert the entire function to pep7, or stick to the existing style. Which should it be? Also, it is a bit hard to try to follow disagreeing opinions of reviewers.

encukou · 2024-03-20T13:53:03Z

Closing; see the issue for details. Adding key to operator.indexOf would be a better way.

kristjanvalur added 2 commits January 6, 2024 14:10

Add the 'key' argument to list.index

9b5aa09

lint

ffa2ed5

kristjanvalur changed the title ~~gh-NNNNN: add list.index() "key" named argument~~ gh-113773: add list.index() "key" named argument Jan 6, 2024

bedevere-app Bot mentioned this pull request Jan 6, 2024

Add 'key' argument to list.index() #113773

Closed

kristjanvalur marked this pull request as ready for review January 6, 2024 14:33

bedevere-app Bot added the awaiting review label Jan 6, 2024

Remove test for no-kw-args for list.index()

cbe2eaa

grigoriev-semyon reviewed Jan 6, 2024

View reviewed changes

Eclips4 reviewed Jan 6, 2024

View reviewed changes

Comment thread Objects/listobject.c Outdated

else if (cmp < 0)

return NULL;

}

else

Copy link
Copy Markdown

Member

Eclips4 Jan 6, 2024

Choose a reason for hiding this comment

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

PEP 7

Eclips4 reviewed Jan 6, 2024

View reviewed changes

kristjanvalur added 2 commits January 7, 2024 12:06

perform 'key' comparison in loop for simplicity

bec9b8d

pep 7

3b46fdb

kristjanvalur force-pushed the kristjan/index_key branch from ef051f5 to 3b46fdb Compare January 7, 2024 12:17

kristjanvalur and others added 2 commits January 7, 2024 12:20

pep7 rest of function

2398a99

📜🤖 Added by blurb_it.

c8ec6bf

sobolevn reviewed Jan 7, 2024

View reviewed changes

encukou closed this Mar 20, 2024

		if (!key)
		for (i = start; i < stop && i < Py_SIZE(self); i++) {

Uh oh!

Conversation

kristjanvalur commented Jan 6, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grigoriev-semyon Jan 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eclips4 Jan 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eclips4 commented Jan 6, 2024

Uh oh!

kristjanvalur commented Jan 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sobolevn Jan 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kristjanvalur Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

encukou commented Mar 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kristjanvalur commented Jan 6, 2024 •

edited by bedevere-app Bot

Loading

grigoriev-semyon Jan 6, 2024 •

edited

Loading

Eclips4 Jan 6, 2024 •

edited

Loading

sobolevn Jan 7, 2024 •

edited

Loading

kristjanvalur Jan 8, 2024 •

edited

Loading