Skip to content

Conversation

Gobot1234
Copy link
Contributor

@Gobot1234 Gobot1234 commented Dec 29, 2024

@Gobot1234 Gobot1234 marked this pull request as draft December 29, 2024 21:31
@Gobot1234 Gobot1234 marked this pull request as ready for review December 29, 2024 21:38
METH_O, slice_indices_doc},
{"__reduce__", (PyCFunction)slice_reduce,
METH_NOARGS, reduce_doc},
{"__class_getitem__", Py_GenericAlias, METH_O|METH_CLASS, "See PEP 585"},
Copy link
Contributor

Choose a reason for hiding this comment

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

This docstring seems misleading, as PEP 585 doesn't mention the slice class.

(Maybe PEP 585 could refer to https://typing.readthedocs.io/en/latest/guides/modernizing.html#generics-in-the-typing-module? Right now it links to documentation for __class_getitem__ that on its own isn't a replacement for PEP 585 from a reader's perspective)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed it is maybe a little silly to be putting PEPs in these things when they aren't active docs.

Copy link
Member

Choose a reason for hiding this comment

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

PRs to the PEPs repo improving things like this are welcome!

Copy link
Contributor Author

@Gobot1234 Gobot1234 Dec 29, 2024

Choose a reason for hiding this comment

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

Curious if @AlexWaygood / @JelleZijlstra have opinions on this. This is done in other places. I seem to have time travelled here

Copy link
Member

Choose a reason for hiding this comment

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

I do agree that our docstrings probably also shouldn't be referencing PEPs, which are, as you both note, historical documents recording decisions that were made at one point in time rather than living documentation. But all the other __class_getitem__ docstrings say this currently, so I think it makes sense to follow suit here and change all the __class_getitem__ docstrings to something better all at once as a followup

@brianschubert
Copy link
Member

I think this calls for a whatsnew entry. Something like

* The :class:`slice` type now supports subscription,
  making it a :term:`generic type`.
  (Contributed by James Hilton-Balfe in :gh:`128335`.)

We should probably also add a versionchanged to the docs noting that slice is now a generic type.

@JelleZijlstra
Copy link
Member

This looks good to me but agree it needs to be documented.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants