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

Behavior of the min/max with key=None #78330

Closed
Amper mannequin opened this issue Jul 18, 2018 · 12 comments
Closed

Behavior of the min/max with key=None #78330

Amper mannequin opened this issue Jul 18, 2018 · 12 comments
Labels
3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@Amper
Copy link
Mannequin

Amper mannequin commented Jul 18, 2018

BPO 34149
Nosy @rhettinger, @terryjreedy, @serhiy-storchaka, @Amper
PRs
  • bpo-34149: Behavior of the min/max with key=None #8328
  • 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 2018-07-24.03:59:33.945>
    created_at = <Date 2018-07-18.16:06:09.916>
    labels = ['3.8', 'type-feature', 'library']
    title = 'Behavior of the min/max with key=None'
    updated_at = <Date 2018-07-24.03:59:33.943>
    user = 'https://github.com/Amper'

    bugs.python.org fields:

    activity = <Date 2018-07-24.03:59:33.943>
    actor = 'rhettinger'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-07-24.03:59:33.945>
    closer = 'rhettinger'
    components = ['Library (Lib)']
    creation = <Date 2018-07-18.16:06:09.916>
    creator = 'amper'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 34149
    keywords = ['patch']
    message_count = 12.0
    messages = ['321891', '321907', '321922', '321923', '321930', '321934', '321937', '322061', '322062', '322067', '322276', '322277']
    nosy_count = 5.0
    nosy_names = ['rhettinger', 'terry.reedy', 'bethard', 'serhiy.storchaka', 'amper']
    pr_nums = ['8328']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue34149'
    versions = ['Python 3.8']

    @Amper
    Copy link
    Mannequin Author

    Amper mannequin commented Jul 18, 2018

    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".

    @Amper Amper mannequin added 3.8 only security fixes type-feature A feature request or enhancement labels Jul 18, 2018
    @serhiy-storchaka
    Copy link
    Member

    Accepting None has a drawback. It can hide a bug when the key function unexpectedly becomes None.

    @rhettinger
    Copy link
    Contributor

    +0 Probably users will never care about this but I don't see a downside beyond the small API churn.

    @serhiy-storchaka
    Copy link
    Member

    Whouldn't be better to add operator.identity and use it as the default value instead of None?

    @rhettinger
    Copy link
    Contributor

    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.

    @serhiy-storchaka
    Copy link
    Member

    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].

    @rhettinger
    Copy link
    Contributor

    Serhiy, feel free to reject this PR.

    @terryjreedy
    Copy link
    Member

    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.

    @terryjreedy
    Copy link
    Member

    FWIW, the nsmallest/largest key param was added Dec 2, 2004, before keyword-only parameters.
    4901a1f

    @rhettinger
    Copy link
    Contributor

    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."

    @rhettinger
    Copy link
    Contributor

    New changeset e22072f by Raymond Hettinger (Alexander Marshalov) in branch 'master':
    bpo-34149: Behavior of the min/max with key=None (GH-8328)
    e22072f

    @rhettinger
    Copy link
    Contributor

    Alexander, thanks for the suggestion and patch.

    @rhettinger rhettinger added the stdlib Python modules in the Lib dir label Jul 24, 2018
    @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.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants