Skip to content

Conversation

@thiagowfx
Copy link

Verify these methods exist:

❯ python3
Python 3.9.6 (default, Dec  2 2025, 07:27:58)
[Clang 17.0.0 (clang-1700.6.3.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import heapq
>>> dir(heapq)
['__about__', '__all__', '__builtins__', '__cached__', '__doc__', '__file__', '__loader__', '__name__', '__package__', '__spec__', '_heapify_max', '_heappop_max', '_heapreplace_max', '_siftdown', '_siftdown_max', '_siftup', '_siftup_max', 'heapify', 'heappop', 'heappush', 'heappushpop', 'heapreplace', 'merge', 'nlargest', 'nsmallest']

Arguably the proper fix is:

-def _heapify_max(heap: list[Any], /) -> None: ...  # undocumented
+def _heapify_max(heap: Iterable[_S], /) -> None: ...  # undocumented
+def _heappop_max(heap: Iterable[_S], /) -> _S: ...  # undocumented
+def _heapreplace_max(heap: Iterable[_S], item: _S, /) -> _S: ...  # undocumented

Opt in for consistency with the pre-existing _heapify_max though.

Verify these methods exist:

```
❯ python3
Python 3.9.6 (default, Dec  2 2025, 07:27:58)
[Clang 17.0.0 (clang-1700.6.3.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import heapq
>>> dir(heapq)
['__about__', '__all__', '__builtins__', '__cached__', '__doc__', '__file__', '__loader__', '__name__', '__package__', '__spec__', '_heapify_max', '_heappop_max', '_heapreplace_max', '_siftdown', '_siftdown_max', '_siftup', '_siftup_max', 'heapify', 'heappop', 'heappush', 'heappushpop', 'heapreplace', 'merge', 'nlargest', 'nsmallest']
```

Arguably the proper fix is:

```diff
-def _heapify_max(heap: list[Any], /) -> None: ...  # undocumented
+def _heapify_max(heap: Iterable[_S], /) -> None: ...  # undocumented
+def _heappop_max(heap: Iterable[_S], /) -> _S: ...  # undocumented
+def _heapreplace_max(heap: Iterable[_S], item: _S, /) -> _S: ...  # undocumented
```

Opt in for consistency with the pre-existing `_heapify_max` though.
@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra
Copy link
Member

We don't add private methods just because they exist, only when people actually need them. Are you using them, or merely adding them for completeness?

@thiagowfx
Copy link
Author

Context: I was trying to use a max heap in Python and accidentally(!) discovered _heapify_max because it is included in this file, as it showed up on neovim auto-completion via pyright(?) LSP:

The initial discovery led me to eventually realize that the two methods added to this PR existed, I just found it odd they were not showing up on auto-completion, then I figured out it's because they're not included in this file.

To answer your question: IMHO it doesn't make sense to have only _heapify_max exposed. We should have either:

  • nothing exposed -> delete _heapify_max
  • add the two missing methods for max heaps (=the current PR state)

I do not advocate to add all private methods – for example, _siftdown_max is still not included, and there is no good reason to do so. However, it would make sense to include all max heap methods, given the entrypoint method is exposed – for consistency, not for completeness.

What are your thoughts?

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.

2 participants