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

gh-113746: Fix set and frozenset documents #130822

Closed
wants to merge 2 commits into from

Conversation

koyuki7w
Copy link
Contributor

@koyuki7w koyuki7w commented Mar 4, 2025

Divided the class description into a part about sets and a part about frozensets. Also, cleaned up the explanation of the comparison method.

Fixed gh-113746.


📚 Documentation preview 📚: https://cpython-previews--130822.org.readthedocs.build/

Divided the class description into a part about sets and a part about frozensets.
Also, cleaned up the explanation of the comparison method.

Fixed pythongh-113746.
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I'm not convinced by this. Isn't there a way to not generate a link for frozenset.update actually? (we can probably hotfix it with a custom extension though)

cc @AA-Turner @rhettinger

Comment on lines 4486 to 4489
.. method:: set == other

Test whether every element of each set is contained in the other, that is,
``set <= other and set >= other``.
Copy link
Member

Choose a reason for hiding this comment

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

All objects implement == so it's redundant here IMO. In addition, we explain the differences with set == frozenset as well.

Comment on lines 4540 to 4545
Both :class:`set` and :class:`frozenset` support set to set comparisons. Two
sets are equal if and only if every element of each set is contained in the
other (each is a subset of the other). A set is less than another set if and
only if the first set is a proper subset of the second set (is a subset, but
is not equal). A set is greater than another set if and only if the first set
is a proper superset of the second set (is a superset, but is not equal).
Copy link
Member

Choose a reason for hiding this comment

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

Please keep this one.

x not in s
isdisjoint(other)
issubset(other)
set == other
Copy link
Member

Choose a reason for hiding this comment

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

Remove this one.


Instances of :class:`frozenset` provide the following :class:`set` operations:

.. method:: len(s)
Copy link
Member

Choose a reason for hiding this comment

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

This would render quite ugly IMO. If users use a link to frozenset.isdisjoint in their Sphinx project, then it will go there and then the user wouldn't have any proper description.

@rhettinger
Copy link
Contributor

I'm not convinced by this. Isn't there a way to not generate a link for frozenset.update actually? (we can probably hotfix it with a custom extension though)

I agree. The actual docs are much better as they are now. There is no redundancy and there docs are very clear about that operations are available for sets that do not apply to frozensets.

If anything needs to be "fixed" it is the indexing. The docs themselves are fine.

Marking this as closed. The solution to #113746 needs to be targeted at indexing rather than this indirect solution that makes the long stable documentation worse.

Thanks for the PR but I would rather live with the minor indexing buglet (or have it fixed in some other way) than to damage the set/frozenset documentation which has served users well for two decades.

@rhettinger rhettinger closed this Mar 6, 2025
@skirpichev skirpichev removed their request for review March 6, 2025 04:47
@koyuki7w koyuki7w deleted the doc-fix-1 branch March 6, 2025 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review docs Documentation in the Doc dir skip news
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

set/frozenset methods intermixed in search, wrong results and target page anchor
3 participants