Skip to content

bpo-46646: mention that bytes are accepted in ipaddress docs #31139

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Feb 5, 2022

I've also added a simple doctest case for ip_interface, because it was the only one without it 🙂

https://bugs.python.org/issue46646

@GBeauregard
Copy link
Contributor

It seems the tests are a bit broken right now, but it wasn't obvious to me which commit or CI change broke things when I looked

Comment on lines 42 to 48
Return an :class:`IPv4Address` or :class:`IPv6Address` object depending on
the IP address passed as argument. Either IPv4 or IPv6 addresses may be
the IP address passed as argument.
*address* is a string or integer or bytes representing the IP address.
Either IPv4 or IPv6 addresses may be
supplied; integers less than ``2**32`` will be considered to be IPv4 by default.
A :exc:`ValueError` is raised if *address* does not represent a valid IPv4
or IPv6 address.
Copy link
Contributor

@GBeauregard GBeauregard Feb 5, 2022

Choose a reason for hiding this comment

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

It makes the diff ugly, but maybe it's still worth reflowing these to fit the style in the rest of the file? Is there a convention when making these changes (to not do this)? (would love a resource if you have one)

   Return an :class:`IPv4Address` or :class:`IPv6Address` object depending on
   the IP address passed as argument. *address* is a string or integer or bytes
   representing the IP address. Either IPv4 or IPv6 addresses may be supplied;
   integers less than ``2**32`` will be considered to be IPv4 by default. A
   :exc:`ValueError` is raised if *address* does not represent a valid IPv4 or
   IPv6 address.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have an opinion on that, I will do anything that is considered the best by maintainers 🙂

Copy link
Member

Choose a reason for hiding this comment

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

The feedback I got from this PR was that it's generally preferred to keep the diff on docs PRs as small as possible, even if that means that some lines go way over (or way under) the 80-char PEP8 limit.

Copy link
Member

Choose a reason for hiding this comment

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

If a line really goes way over 80 characters, then split it in two 🙂

Copy link
Contributor

@MaxwellDupre MaxwellDupre left a comment

Choose a reason for hiding this comment

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

The diff doesn't look that bad to me. More important is the finished Docs and they read ok.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 18, 2022

The diff doesn't look that bad to me. More important is the finished Docs and they read ok.

We shouldn't be documenting that bytes are supported for these functions if passing bytes in is completely untested: #90804 (comment).

merwok
merwok previously approved these changes Jun 20, 2022
@sobolevn
Copy link
Member Author

sobolevn commented Nov 14, 2022

@JelleZijlstra should we also document that it also accepts tuple?
Coming back from python/typeshed#8966

@JelleZijlstra
Copy link
Member

Not sure, is tuple support tested in CPython?

If we document it, we should be precise about what the tuple represents.

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.

9 participants