Skip to content

/ cpython

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

bpo-12159: document sys.maxsize limit in len() function reference #17934

Open
wants to merge 1 commit into
base: master
from
Open

bpo-12159: document sys.maxsize limit in len() function reference #17934

wants to merge 1 commit into from
+4 −0

Conversation

@Zac-HD
Copy link
Contributor

Zac-HD commented Jan 10, 2020 •
edited by bedevere-bot

See:

  • BPO-12159: "needs to be documented",
  • BPO-15718: added note to datamodel docs for __len__ method but not len(), and
  • BPO-21444: change request / wontfix

https://bugs.python.org/issue12159
@cool-RR

This comment has been minimized.

Sign in to view
Copy link
Contributor

cool-RR commented Jan 10, 2020

I think that this is a good addition to the documentation, but the example with range is weird. Maybe you mean that big ranges, such as range(10 ** 100) will be too big. That isn't clear from your text.
@vstinner
vstinner reviewed Jan 10, 2020
View changes
Doc/library/functions.rst Outdated
@@ -892,6 +892,9 @@ are always available. They are listed here in alphabetical order.
sequence (such as a string, bytes, tuple, list, or range) or a collection
(such as a dictionary, set, or frozen set).

**CPython implementation detail:** ``len`` raises :class:`OverflowError`
on inputs larger than :data:`sys.maxsize`, such as :class:`range`.

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner Jan 10, 2020

Member
Suggested change
on inputs larger than :data:`sys.maxsize`, such as :class:`range`.
on lengths larger than :data:`sys.maxsize`.

I don't understand the "such as range()" part. len(range(5)) returns 5, it doesn't fail with overflow?

This comment has been minimized.

Sign in to view
Copy link
@Zac-HD

Zac-HD Jan 11, 2020

Author Contributor

I've clarified that this is for cases such as range(2 ** 64).
@Zac-HD Zac-HD force-pushed the Zac-HD:document-len-overflow branch from 9837fa4 to 73051ca Jan 11, 2020
@vstinner
vstinner reviewed Jan 11, 2020
View changes
Doc/library/functions.rst Outdated Show resolved Hide resolved
@Zac-HD
Note size limit for len()
Loading status checks…
b62dc23
@Zac-HD Zac-HD force-pushed the Zac-HD:document-len-overflow branch from 73051ca to b62dc23 Jan 11, 2020
@vstinner vstinner changed the title BPO-12159: document `sys.maxsize` limit in function reference for `len()` bpo-12159: document sys.maxsize limit in len() function reference Jan 11, 2020
@vstinner vstinner added the skip news label Jan 11, 2020
@vstinner
vstinner approved these changes Jan 11, 2020
View changes
Copy link
Member

vstinner left a comment

LGTM, but I would prefer to get a second review. @csabella @serhiy-storchaka @JulienPalard: What do you think of this len note?
@csabella
csabella approved these changes Jan 12, 2020
View changes
Copy link
Contributor

csabella left a comment

LGTM. Since this has been brought up multiple times, I think it's best to document it, especially since __len__ had a note added in the past. Thanks for the patch, @Zac-HD!
@csabella
csabella requested changes Jan 12, 2020
View changes
Doc/library/functions.rst
@@ -892,6 +892,10 @@ are always available. They are listed here in alphabetical order.
sequence (such as a string, bytes, tuple, list, or range) or a collection
(such as a dictionary, set, or frozen set).

**CPython implementation detail:** ``len`` raises :exc:`OverflowError`

This comment has been minimized.

Sign in to view
Copy link
@csabella

csabella Jan 12, 2020

Contributor

There is a directive for implementation details:

.. impl-detail::
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Jan 12, 2020

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csabella csabella

@vstinner vstinner

Assignees
No one assigned
Labels
CLA signed awaiting changes skip news type-documentation
Projects
None yet
Milestone
No milestone
6 participants
@Zac-HD @cool-RR @bedevere-bot @vstinner @csabella @the-knights-who-say-ni
You can’t perform that action at this time.