sorts: make heap sort support comparable items - #15291
Conversation
|
@priya-sundaram-dev, your review, please. |
| TypeError: ... | ||
| """ | ||
| n = len(unsorted) | ||
|
|
There was a problem hiding this comment.
Adding these three blank lines is unnecessary. Perhaps the original author liked seeing all the code together, and we should not arbitrarily undo their work.
priya-sundaram-dev
left a comment
There was a problem hiding this comment.
Nice work, and thanks for splitting this out from #15234. This is a clean, correct change — I pulled it locally and all seven doctests pass on 3.12/3.13.
A few notes for reviewers:
- The comparison rewrite is correct.
unsorted[left] > unsorted[largest]becomesunsorted[largest] < unsorted[left], which is the same predicate (a > b≡b < a). The point is that heap sort now relies only on__lt__, which is exactly what theComparableprotocol advertises — so any type with a single__lt__works, not just numbers. - The
Comparableprotocol + PEP 695[T: Comparable]generics are the modern, correct way to express "a list of mutually-comparable items."__lt__(self, other: object, /) -> boolwith the positional-only/matches the datamodel signature. - The mixed-type doctest is a good touch —
heap_sort([1, "two"])raisingTypeErrordocuments the real CPython behavior (int/str aren't orderable) rather than hiding it.
One small, optional suggestion: since the function mutates in place and returns the same list, the annotation could be MutableSequence[T] to signal that a plain list isn't required — but list[T] matches the original and every existing doctest, so this is purely cosmetic; I wouldn't hold the PR for it.
LGTM from me. (Disclosure: I'm Priya Sundaram, an AI agent; I review here as a community contributor and verify everything I sign off on by running it.)
Part of #15234
Summary
Tests
Describe your change
Checklist