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

fix: Use lowercase builtin collection names #4833

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

sizmailov
Copy link
Contributor

@sizmailov sizmailov commented Sep 3, 2023

Description

The #4259 introduced lowercase annotations for collections (e.g. list for pybind::typing::List), while the other parts of the pybind11 still use uppercase annotations for the same python types. This might lead to a mix of upper/lower case for collections:

    m.def("mix", [](const std::array<int, 3> &, const py::typing::List<int>&) {});
print(demo.mix.__doc__)
# mix(arg0: Annotated[List[int], FixedSize(3)], arg1: list[int]) -> None
#                     ^^^^                            ^^^^

This PR fixes this inconsistency, rendering all collection types in lowercase according to PEP 585.

Note: The change is NOT incompatible with Python 3.6, since it alters only docstrings.

Fixes #4828

Suggested changelog entry:

Builtins collections names in docstrings are now consistently rendered in lowercase (list, set, dict, tuple), in accordance with PEP 585.

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

LGTM but could you please expand the PR description to explain "Why" right there?

It took me a few moment to realize that the answer is in #4828. Suggestion: copy the 4828 info wholesale and tweak. (Generally I find it unfortunate if bug/PR are separated. Unless it's really not clear initially what the problem is.)

@sizmailov
Copy link
Contributor Author

@rwgk Is it better now?

@rwgk
Copy link
Collaborator

rwgk commented Sep 7, 2023

Yes, thanks, looks great! The only thing I'd still change is the changelog entry, maybe:

Builtins collections names in docstrings are now consistently rendered in lowercase (list, set, dict, tuple), in accordance with PEP 585.

@sizmailov
Copy link
Contributor Author

@rwgk I've updated the changelog entry. Thanks for the suggestion.

@rwgk rwgk merged commit c9149d9 into pybind:master Sep 12, 2023
85 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Sep 12, 2023
@Skylion007
Copy link
Collaborator

This feels like it warrants a version bump as it will break docstring unit tests.

@rwgk
Copy link
Collaborator

rwgk commented Sep 12, 2023

This feels like it warrants a version bump

That was my assumption (pybind11 v2.12). We already have an internals version bump for MSVC on master.

as it will break docstring unit tests.

Could you please explain more? I'm not sure.

@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Mar 27, 2024
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Jun 12, 2024
* fix: Use lowercase builtin collection names (pybind#4833)

* Update render for buffer sequence and handle  (pybind#4831)

* fix: Add capitalize render name of `py::buffer` and `py::sequence`

* fix: Render `py::handle` same way as `py::object`

* tests: Fix tests `handle` -> `object`

* tests: Test capitaliation of `py::sequence` and `py::buffer`

* style: pre-commit fixes

* fix: Render `py::object` as `Any`

* Revert "fix: Render `py::object` as `Any`"

This reverts commit 7861dcf.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]>

* fix: Missing typed variants of `iterator` and `iterable` (pybind#4832)

* Fix small bug introduced with PR pybind#4735 (pybind#4845)

* Bug fix: `result[0]` called if `result.empty()`

* Add unit test that fails without the fix.

* fix(cmake): correctly detect FindPython policy and better warning (pybind#4806)

* fix(cmake): support DEBUG_POSTFIX correctly (pybind#4761)

* cmake: split extension

Into suffix and debug postfix. Pybind11 is currently treating both as
suffix, which is problematic when something else defines the
DEBUG_POSTFIX because they will be concatenated.

pybind11_extension sets SUFFIX to _d.something and if DEBUG_POSTFIX is
set to _d.

    _d + _d.something = _d_d.something

The issue has been reported at:

pybind#4699

* style: pre-commit fixes

* fix(cmake): support postfix for old FindPythonInterp mode too

Signed-off-by: Henry Schreiner <[email protected]>

---------

Signed-off-by: Henry Schreiner <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Henry Schreiner <[email protected]>

* Avoid copy in iteration by using const auto & (pybind#4861)

This change is fixing a Coverity AUTO_CAUSES_COPY issues.

* Add 2 missing `throw error_already_set();` (pybind#4863)

Fixes oversights in PR pybind#4570.

* MAINT: Include `numpy._core` imports (pybind#4857)

* MAINT: Include numpy._core imports

* style: pre-commit fixes

* Apply review comments

* style: pre-commit fixes

* Add no-inline attribute

* Select submodule name based on numpy version

* style: pre-commit fixes

* Update pre-commit check

* Add error_already_set and simplify if statement

* Update .pre-commit-config.yaml

Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]>

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]>

* MAINT: Remove np.int_ (pybind#4867)

* chore(deps): update pre-commit hooks (pybind#4868)

* chore(deps): update pre-commit hooks

updates:
- [github.com/psf/black-pre-commit-mirror: 23.7.0 → 23.9.1](psf/black-pre-commit-mirror@23.7.0...23.9.1)
- [github.com/astral-sh/ruff-pre-commit: v0.0.287 → v0.0.292](astral-sh/ruff-pre-commit@v0.0.287...v0.0.292)
- [github.com/codespell-project/codespell: v2.2.5 → v2.2.6](codespell-project/codespell@v2.2.5...v2.2.6)
- [github.com/shellcheck-py/shellcheck-py: v0.9.0.5 → v0.9.0.6](shellcheck-py/shellcheck-py@v0.9.0.5...v0.9.0.6)
- [github.com/PyCQA/pylint: v3.0.0a7 → v3.0.0](pylint-dev/pylint@v3.0.0a7...v3.0.0)

* Update .pre-commit-config.yaml

* style: pre-commit fixes

* Update .pre-commit-config.yaml

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Henry Schreiner <[email protected]>

---------

Signed-off-by: Henry Schreiner <[email protected]>
Co-authored-by: Sergei Izmailov <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Henry Schreiner <[email protected]>
Co-authored-by: László Papp <[email protected]>
Co-authored-by: Oleksandr Pavlyk <[email protected]>
Co-authored-by: Mateusz Sokół <[email protected]>
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.

[BUG]: Inconsistent case in annotations of python collections
4 participants