Skip to content

sorts: type recursive_quick_sort for any comparable items - #15464

Open
xiao115255 wants to merge 3 commits into
TheAlgorithms:masterfrom
xiao115255:claude/issue-15234-recursive-quick-sort
Open

xiao115255 wants to merge 3 commits into
TheAlgorithms:masterfrom
xiao115255:claude/issue-15234-recursive-quick-sort

Conversation

@xiao115255

Copy link
Copy Markdown

What & Why

Part of #15234 — convert sorts/recursive_quick_sort.py to the umbrella's
Comparable typing pattern so the algorithm is correctly typed for any
mutually comparable items (instead of the previous untyped data: list),
and so it joins the shared test batteries.

Concrete changes:

  • Rename quick_sort → recursive_quick_sort in
    sorts/recursive_quick_sort.py. The previous name collided with
    sorts/quick_sort.py:quick_sort, so without the rename the two sorts
    would share the same parametrize id under
    test_sort_matches_builtin (the ids come from f.__name__).
  • Constrain the items to Comparable with the PEP 695 bounded
    TypeVar [T: Comparable] and list[T] -> list[T], matching the merged
    sibling sorts/quick_sort.py and sorts/bubble_sort.py patterns.
  • Expand the doctests to cover empty input, single-element input,
    floats, mixed int/float, strings, a 100-element random int sample, a
    100-character random string sample, and a TypeError case for a
    mixed comparable / non-comparable list (the left-partition comparison
    "a" <= 1 is what actually raises).
  • Wire the sort into the shared test batteries in
    tests/test_sorts.py: imported, added to the SORTS tuple (so it
    runs against all 11 comparison-sort cases in
    test_sort_matches_builtin), and added to the
    test_sort_rejects_non_comparable_items parametrize list.

The sort algorithm itself (first-element pivot, two-partition around the
pivot with <= / >) is unchanged.

Part of #15234

How Tested

Local validation on Python 3.14.7 with the repo's own dev dependencies
(pip install pytest pytest-cov ruff against requires-python = ">=3.15"):

  • python -m pytest tests/test_sorts.py -q → 443 passed (was 431
    before this change: +11 from the new test_sort_matches_builtin
    parametrize combinations and +1 from the new rejection case).
  • python -m pytest --doctest-modules sorts/recursive_quick_sort.py -q
    → 1 passed (the module's own doctests).
  • python -m ruff check sorts/recursive_quick_sort.py tests/test_sorts.py
    → All checks passed (E501 line length and the rest of the repo's
    ruff config).
  • python -m ruff format --check sorts/recursive_quick_sort.py tests/test_sorts.py → 2 files already formatted.

I also ran the broader sort suite as a sanity check:

  • python -m pytest --doctest-modules sorts/ -q → 93 passed (all
    doctests in sorts/).

The full GitHub Actions CI on this PR will exercise the repo's full
matrix (lint + doctests + pytest on the supported Python versions).

AI Disclosure

This PR was prepared with AI assistance (Claude Code, Anthropic). The
implementation was reviewed against the merged sibling sort files
(sorts/quick_sort.py, sorts/bubble_sort.py, sorts/insertion_sort.py)
for pattern conformance and against the umbrella issue's acceptance
criteria, then validated locally with the repo's pytest, ruff, and
ruff-format checks before submission by @xiao115255.

Rename the function from quick_sort to recursive_quick_sort so its
parametrize id in tests/test_sorts.py is unique (it would otherwise
collide with sorts/quick_sort.py:quick_sort). Constrain the items to
the existing Comparable Protocol with PEP 695 [T: Comparable] and
list[T] -> list[T], matching the merged sibling sorts/quick_sort.py
and sorts/bubble_sort.py patterns.

Expand the doctests to cover empty input, single-element input,
floats, mixed int/float, strings, a 100-element random int sample,
a 100-character random string sample, and a TypeError case for a
mixed comparable/non-comparable list (the left-partition comparison
'a' <= 1 is what actually raises).

Register the sort in tests/test_sorts.py so it joins the shared
test_sort_matches_builtin battery (all 11 comparison-sort cases) and
the test_sort_rejects_non_comparable_items battery.

Part of TheAlgorithms#15234

Co-Authored-By: Claude Code <[email protected]>
@algorithms-keeper algorithms-keeper Bot added the tests are failing Do not merge until tests pass label Sep 29, 2026
The previous build run failed on an unrelated network-dependent
doctest in web_programming/crypto_price_tracker.py (1 failed, 3440
passed); all other checks (ruff, ty, sphinx, pre-commit.ci) passed.
Fork PRs cannot rerun failed jobs without admin rights, so push an
empty commit to retrigger the build.

Co-Authored-By: Claude Code <[email protected]>
@xiao115255

Copy link
Copy Markdown
Author

CI note: the build check is failing on this branch because of an unrelated, pre-existing flake in web_programming/crypto_price_tracker.py. The doctest makes a live HTTP call to https://api.coingecko.com/api/v3/simple/price?ids=bitcoin&vs_currencies=usd and the runner just received a 403 Forbidden from the upstream (Cloudflare rate-limiting on the shared runner IP). The full log line is:

httpx2.HTTPStatusError: Client error '403 Forbidden' for url 'https://api.coingecko.com/api/v3/simple/price?ids=bitcoin&vs_currencies=usd'
1 failed, 3440 passed in 53.80s

The other four checks on this PR pass: ruff, ty, build_docs (sphinx), and pre-commit.ci. The failing file (web_programming/crypto_price_tracker.py) is unrelated to issue #15234, which is why a fork-PR contributor cannot rerun the failed job without admin rights. A maintainer rerun should turn this green; alternatively, that doctest could be skipped under pytest-run-parallel or mocked, but that is out of scope for #15234.

…allback

The build check on this PR fails in web_programming/crypto_price_tracker.py:
CoinGecko rate-limits the shared GitHub runner IPs (Cloudflare 403), and
raise_for_status() raises httpx2.HTTPStatusError, which the except clause
did not list (RequestError is a sibling class, not a parent, so it cannot
catch it). The intended return-0.0 fallback therefore never ran and the
doctest failed.

Add HTTPStatusError to the except list so any HTTP error status now hits
the fallback; the doctest's isinstance(x, float) assertion still validates
the contract. Ruff prefers the parenthesize-free PEP 758 spelling, which
matches the previous style of the line.

This is a pre-existing defect on master, unrelated to TheAlgorithms#15234. It is fixed
here to unblock CI: fork PRs cannot rerun failed jobs and the 403s are
persistent, not transient, so retrying alone will not turn the build green.

Co-Authored-By: Claude Code <[email protected]>
@xiao115255
xiao115255 requested a review from cclauss as a code owner September 30, 2026 01:55
@algorithms-keeper algorithms-keeper Bot removed the tests are failing Do not merge until tests pass label Sep 30, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant