Skip to content

bpo-23747: Add small note to document default method behavior #31462

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

Merged
merged 6 commits into from
Apr 20, 2022

Conversation

slateny
Copy link
Contributor

@slateny slateny commented Feb 21, 2022

Not too sure on the convention for whether the two notes should be bunched together into one or kept separate. Also not too sure on the difference between using :func: vs :meth: as both seem to resolve to the same link in the end

https://bugs.python.org/issue23747

@slateny slateny changed the title bpo-23747: Added small note to document default method behavior bpo-23747: Add small note to document default method behavior Mar 4, 2022
.. note::

Calling a platform-specific method on a mismatched platform will return their
default values, such as a tuple with empty string values.
Copy link
Member

Choose a reason for hiding this comment

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

It's hard to give a global rule for all platform functions. Default values:

  • win32_edition() returns None
  • win32_is_iot() returns a boolean
  • freedesktop_os_release() has no default value, it raises an OSError if the file is missing
  • processor() returns an empty string

To be less misleading, maybe you can name a function which returns a tuple of empty strings.

But it's strange to have a global rule saying "by default, return the default value"... but don't actually document the default value of each function... I'm not sure that this addition is helpful.

platform.version() documents its default value. Maybe it's an example to follow?

An empty string is returned if the value cannot be determined.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding of the bpo is that the return values for these functions are a bit atypical, and as such we want to make note of it so users pay a bit closer attention to the platform and the functions they're using, so maybe we can use weaker language and say 'Calling some platform-specific functions on a mismatched platform may return their default values, such as a tuple with empty string values'.

As for documenting its default value, would all functions be documented with their return values for mismatched platforms? That seems like it might add too much unneeded information and add to maintainability, but it would definitely avoid confusion. I don't feel too strongly either way as both methods seem fine to me, so I'd differ to you on which choice is better here.

Copy link
Member

Choose a reason for hiding this comment

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

Documenting default value of each function sounds more correct to me than a general note which is either incorrect (if you are too specific about the default value) or unclear (don't say anything about the default value).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the platform-specific functions on the platform page, win32_ver and win32_edition seem to be the only two functions with unclear return values in the case of using on mismatched platform, so I threw in some documentation for their return values there (and some slight formatting changes above).

I also see that java_ver has this: Values which cannot be determined are set to the defaults given as parameters (which all default to ''), but I'm wondering whether that's correct as vminfo defaults to ('', '', '') instead. Maybe it should say something like ... (which default to an empty string or a tuple of empty strings) or ... (which default to '' or a tuple of '')

Co-authored-by: Victor Stinner <[email protected]>
@vstinner vstinner merged commit ad3ca17 into python:main Apr 20, 2022
@vstinner
Copy link
Member

Merged, thanks.

@vstinner vstinner added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Apr 20, 2022
@miss-islington
Copy link
Contributor

Thanks @slateny for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @slateny for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry @slateny and @vstinner, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker ad3ca17ff5cd63f907430073b52be27695674148 3.10

@bedevere-bot
Copy link

GH-91753 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Apr 20, 2022
@vstinner vstinner added needs backport to 3.10 only security fixes and removed needs backport to 3.10 only security fixes labels Apr 20, 2022
@miss-islington
Copy link
Contributor

Thanks @slateny for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-91754 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Apr 20, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 20, 2022
miss-islington added a commit that referenced this pull request Apr 20, 2022
miss-islington added a commit that referenced this pull request Apr 20, 2022
@slateny slateny deleted the s/platform branch April 26, 2022 05:42
hello-adam pushed a commit to hello-adam/cpython that referenced this pull request Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants