Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-129622: Clarify theset.is{sub,super}set docstrings#129637

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
AA-Turner wants to merge2 commits intopython:main
base:main
Choose a base branch
Loading
fromAA-Turner:set-docstring

Conversation

AA-Turner
Copy link
Member

@AA-TurnerAA-Turner commentedFeb 4, 2025
edited by bedevere-appbot
Loading

@@ -2066,12 +2066,12 @@ set.issuperset
other: object
/

Report whether thissetcontains another set.
Return True if thesetis a superset of other.
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, the OP issue is about return value. I think we can replace "report" with "Return True if", but why change the rest? Superset/subset - terminology, that might be unfamiliar to readers.

merwok reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

My specific suggestion was "Return True ifself is a subset ofother, else False.", with changing 'Report' with 'Return' the prime focus. I do not know if 'if else' is needed. Without looking around, I suspect both implicit and explicit alternatives are used and would be satified either way.

I used the parameter names (marked as italicized) as that is a general policy. For frozenset, the parameter names are used in all the dunder docstrings (but without *s) but never with the non-dunder functions. The look sloppy to me. The non-specific 'another set' seems weird to me compared to the specific 'other'. As for the superset relationship , the rewording of the repetitious 'is superset' as 'contains' (and similarly for 'is subset') is fine with me and even a good idea. My revised suggestion would then be "Return True ifself containsother." and "Return True ifother containsself." (both without or with ", else False")

Copy link
MemberAuthor

@AA-TurnerAA-TurnerFeb 4, 2025
edited
Loading

Choose a reason for hiding this comment

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

As far as I can tell, none of the docstrings insetobject.c use 'self'. Other docstrings in the module referenceself as "a" / "this" / "the" set. Perhaps?:

Return True if this set contains other. andReturn True if this set is contained by other.

My slight preference for {sub/super}set is as "contains" is ambiguous for the case of set equality, where both methods returnTrue.

@terryjreedy
Copy link
Member

terryjreedy commentedFeb 4, 2025
edited
Loading

Test failure (Check if generated files are up to date) is build failure of python3.13 ./Tools/build/generate_sbom.py with messageRemoteDisconnected("Remote end closed connection without response"). Seems like random failure; rerunning the one test. EDIT: rerun passed.

@rhettingerrhettinger self-assigned thisFeb 4, 2025
@rhettinger
Copy link
Contributor

Thoughts:

  • There isn't a real issue here. Python has a bunch ofis... predicates and people seem to get that they all return booleans. AFAICT no actual user has ever had a problem with knowing that any these methods return a boolean.

  • Some of the predicates likestr.isupper are worded in the form "Return True if ..., False otherwise". Others likeisinstance are worded in the form "Return whether ...". Either form seems to suffice.

  • It would be step backwards to replace "another set contains this set" with "if the set is a subset of other." The latter is a circular definition and add no value. Consider possible docstrings foris_coprime(p, q). One says "returns True if p is coprime to q". Another says "Returns True is p has no factors in common with q". One just repeats the functions name and the other teaches you something about what it does.

  • If some change has to be made, then let's address an actual user problem. I've seen confusion about whetherp.issubset(q) means thatp is a subset ofq or whether it means thatq is a subset ofp. That said, I think the confusion is intrinsic to the word order of the call itself. So there isn't much than can be done. Thankfully, people mostly just use thes < t notation which is unambigous.

Sabfo reacted with thumbs up emoji

@rhettingerrhettinger added the pendingThe issue will be closed if no feedback is provided labelFeb 5, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@skirpichevskirpichevskirpichev left review comments

@terryjreedyterryjreedyAwaiting requested review from terryjreedy

@rhettingerrhettingerAwaiting requested review from rhettingerrhettinger is a code owner

Assignees

@rhettingerrhettinger

Labels
awaiting core reviewpendingThe issue will be closed if no feedback is providedskip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@AA-Turner@terryjreedy@rhettinger@skirpichev@picnixz

[8]ページ先頭

©2009-2025 Movatter.jp