Skip to content

feat(sorts): make reversort generic over comparable items - #15402

Merged
cclauss merged 3 commits into
TheAlgorithms:masterfrom
5h4d0wn1k:sorts/make-reversort-generic
Sep 22, 2026
Merged

cclauss merged 3 commits into
TheAlgorithms:masterfrom
5h4d0wn1k:sorts/make-reversort-generic

Conversation

@5h4d0wn1k

Copy link
Copy Markdown
Contributor

Describe your change

Part of #15234

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Add or change doctests?
  • Documentation change?

Make reversort (and its reversort_cost helper) sort any mutually comparable items instead of only list[int]:

  • Switched list[Any] to the shared Comparable Protocol with a TypeVar bound, as in the reference insertion_sort.py
  • Added a TypeError doctest to both functions confirming mixed non-comparable input raises instead of silently mis-sorting
  • Registered reversort in tests/test_sorts.py so it is exercised against the shared battery (ints, floats, strings, Person/Dog dataclasses) and the non-comparable rejection path

Checklist

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.

Part of TheAlgorithms#15234

- Replace list[Any] with a Comparable-bounded TypeVar so reversort and
  reversort_cost sort any mutually comparable items, not just ints
- Add TypeError doctests confirming mixed non-comparable input raises
- Register reversort in the shared test battery covering str/float/
  dataclass/NamedTuple cases and the non-comparable rejection path
@algorithms-keeper algorithms-keeper Bot added awaiting reviews This PR is ready to be reviewed enhancement This PR modified some existing files tests are failing Do not merge until tests pass labels Sep 22, 2026
ruff UP047 requires the sanctioned [T: Comparable] signature form used
by the reference insertion_sort.py. Keep the module-level TypeVar as in
the reference.
@cclauss

cclauss commented Sep 22, 2026

Copy link
Copy Markdown
Member

ON HOLD: Our focus is on merging or closing old pull requests before October 1st.

@cclauss

cclauss commented Sep 22, 2026

Copy link
Copy Markdown
Member

@priya-sundaram-dev, please review. Are the type hints correct?

@algorithms-keeper algorithms-keeper Bot removed the tests are failing Do not merge until tests pass label Sep 22, 2026

@priya-sundaram-dev priya-sundaram-dev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The type hints are correct, and the Comparable protocol with just __lt__ is exactly the right minimal bound for a sort — nicely done. Two small things:

  1. Redundant TypeVar. You've written the bound twice: once as the module-level T = TypeVar("T", bound=Comparable) and once inline via PEP 695 (def reversort[T: Comparable](...)). The inline form shadows the module-level one, so the T = TypeVar(...) line is dead. Since this repo targets the latest CPython, I'd keep the PEP 695 syntax and drop both the TypeVar import and the assignment — leaving from typing import Any, Protocol.

  2. Doctests look right. +IGNORE_EXCEPTION_DETAIL is the correct call here, since the TypeError message can read 'str' and 'int' or 'int' and 'str' depending on which comparison trips first — ignoring the detail keeps it deterministic across runs.

With the redundant TypeVar removed this is good to go. mypy and ruff should both be happy after that.

Comment thread sorts/reversort.py Outdated
Comment thread sorts/reversort.py Outdated
@algorithms-keeper algorithms-keeper Bot added awaiting changes A maintainer has requested changes to this PR and removed awaiting reviews This PR is ready to be reviewed labels Sep 22, 2026
@priya-sundaram-dev

Copy link
Copy Markdown
Contributor

The type hints look correct to me, and they match the reference sorts/insertion_sort.py faithfully:

  • class Comparable(Protocol) with __lt__ and the PEP 695 def reversort[T: Comparable](collection: list[T]) -> list[T] bound are exactly the shared pattern insertion_sort.py uses, and py314 (the repo target) supports the inline syntax. reversort only relies on <, so a Comparable bound is the right constraint — nothing here needs full functools.total_ordering.
  • Using list[T] (rather than MutableSequence[T]) is fine: reversort does collection[:] then index-assigns, and every doctest passes a concrete list, so the narrower type is accurate.
  • Good instinct using # doctest: +IGNORE_EXCEPTION_DETAIL on the [1, "a"] cases — that keeps the doctest robust to the exact '<' not supported between instances of ... wording (which can differ in operand order between runs).

One tiny, optional nit — not blocking: with the PEP 695 [T: Comparable] in the signature, the module-level T = TypeVar("T", bound=Comparable) is redundant (the inline form introduces its own scoped T). insertion_sort.py keeps that line too, so leaving it in is consistent with the codebase; if you'd rather drop the dead line you could also drop TypeVar from the imports. Either way ruff (py314) stays happy. LGTM.

Co-authored-by: Christian Clauss <cclauss@me.com>
@algorithms-keeper algorithms-keeper Bot added awaiting reviews This PR is ready to be reviewed and removed awaiting changes A maintainer has requested changes to this PR labels Sep 22, 2026
@algorithms-keeper algorithms-keeper Bot removed the awaiting reviews This PR is ready to be reviewed label Sep 22, 2026
@cclauss
cclauss merged commit 763b146 into TheAlgorithms:master Sep 22, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement This PR modified some existing files on hold

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants