classification
Title: Behavior of the min/max with key=None
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amper, bethard, rhettinger, serhiy.storchaka, terry.reedy
Priority: normal Keywords: patch

Created on 2018-07-18 16:06 by amper, last changed 2018-07-24 03:59 by rhettinger. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 8328 merged amper, 2018-07-18 16:09
Messages (12)
msg321891 - (view) Author: Alexander Marshalov (amper) * Date: 2018-07-18 16:06
I was faced with the fact that the behavior of the functions "min"/"max" and "sorted" is a little different.

For example, this code works fine:

    >>> sorted([3, 2, 1], key=None)
    [1, 2, 3]

But the same example for "min" and "max" doesn't work:

    >>> min([3, 2, 1], key=None)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: 'NoneType' object is not callable

That is why the heapq library has this code:

    ...
    def nsmallest(n, iterable, key=None):
        ...
            if key is None:
                result = min(it, default=sentinel)
            else:
                result = min(it, default=sentinel, key=key)
        ...

At the same time, there are many places where such checks are not performed for the "sorted" function. I think the behavior of the "min" / "max" / "sorted" functions should be unified. That is, they should work as if "None" is the default value for "key".
msg321907 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-18 17:40
Accepting None has a drawback. It can hide a bug when the key function unexpectedly becomes None.
msg321922 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2018-07-19 05:18
+0 Probably users will never care about this but I don't see a downside beyond the small API churn.
msg321923 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-19 05:22
Whouldn't be better to add operator.identity and use it as the default value instead of None?
msg321930 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2018-07-19 06:34
> Whouldn't be better to add operator.identity and use it as the default value instead of None?

No, that would incur overhead that currently isn't present.
msg321934 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-19 07:38
Accepting None makes the typing model more complex. Instead of just a callable functions accept callable-or-none. It terms of annotations, it is Union[Callable[[Any], Any], None] instead of just Callable[[Any], Any].
msg321937 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2018-07-19 08:31
Serhiy, feel free to reject this PR.
msg322061 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-07-21 02:17
In 2.x, map(None, 'abc', 'zyz') == [('a', 'z'), ('b', 'y'), ('c', 'z')], but with the addition of zip, so that zip('abc', 'xyz') has the same result, we deprecated that use of None to mean identity function.

For python-coded functions, a default is needed to make a keyword-only argument optional, and preferred over use of *args for making positional arguments optional.  Unless I am missing something, a function can special-case 'key is identity', to avoid overhead, just as well as it can special-case 'key is None'.  So rather than extend 'key=None', to me a kludge, I would rather replace it with 'key=identity'.  Both can be accepted during a deprecation period.

For instance, after adding identity,

def nsmallest(n, iterable, key=identity):
    ...
        if key is identity or key is None:  # key in (identity, None)
            result = min(it, default=sentinel)
    ...

Since no one need ever write key=None, explicit passing should be rare.  It seems to me that the main reason for the type declaration of key to include None is so that the def statement itself passes a consistency check with the None default.  Once that is changed, most people should be able to use a simple callable declaration.  I am considering this for python-ideas.

Since the weekly issues list came out just 10 hours ago, I will not close this yet, but I will if still open in couple of days and no coredev objections.
msg322062 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-07-21 02:20
FWIW, the nsmallest/largest key param was added Dec 2, 2004, before keyword-only parameters.
https://github.com/python/cpython/commit/4901a1f267e9d632f85054ce8b21ff23bff305e1
msg322067 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2018-07-21 03:01
Repeating my comment on the Github PR, "I'm persuaded by your idea that min(), max(), nsmallest(), nlargest(), sorted(), and itertools.groupby() should ideally have the same API for key functions. Syncing of those APIs would be a minor improvement. As Serhiy pointed out, this does complicate the type signature a bit, but that is of small concern giving that the other four functions listed have already gone down this path."
msg322276 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2018-07-24 03:58
New changeset e22072fb11246f125aa9ff7629c832b9e2407ef0 by Raymond Hettinger (Alexander Marshalov) in branch 'master':
bpo-34149: Behavior of the min/max with key=None (GH-8328)
https://github.com/python/cpython/commit/e22072fb11246f125aa9ff7629c832b9e2407ef0
msg322277 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2018-07-24 03:59
Alexander, thanks for the suggestion and patch.
History
Date User Action Args
2018-07-24 03:59:33rhettingersetstatus: open -> closed
messages: + msg322277

components: + Library (Lib)
resolution: fixed
stage: patch review -> resolved
2018-07-24 03:58:28rhettingersetmessages: + msg322276
2018-07-21 03:01:31rhettingersetmessages: + msg322067
2018-07-21 02:20:34terry.reedysetmessages: + msg322062
2018-07-21 02:17:49terry.reedysetnosy: + terry.reedy
messages: + msg322061
2018-07-19 08:31:30rhettingersetmessages: + msg321937
2018-07-19 07:38:04serhiy.storchakasetmessages: + msg321934
2018-07-19 06:34:11rhettingersetmessages: + msg321930
2018-07-19 05:22:50serhiy.storchakasetmessages: + msg321923
2018-07-19 05:18:54rhettingersetmessages: + msg321922
2018-07-18 17:40:00serhiy.storchakasetnosy: + rhettinger, serhiy.storchaka, bethard
messages: + msg321907
2018-07-18 16:09:11ampersetkeywords: + patch
stage: patch review
pull_requests: + pull_request7864
2018-07-18 16:06:09ampercreate