gh-83791: Raise TypeError for len(memoryview_0d)#18463
Conversation
530d5f3 to
5200b72
Compare
There was a problem hiding this comment.
This if was a pointless special case
There was a problem hiding this comment.
This if always evaluated to False, so the garbage code within it never ran. Fixed to be identical to the above.
5200b72 to
b897e68
Compare
This makes `memoryview.__len__` consistent with `memoryview.__getitem__`. Also activates and fixes a bug in dead code in the `test_endian` test, which was only being run on 0d arrays.
b897e68 to
c210029
Compare
Codecov Report
@@ Coverage Diff @@
## master #18463 +/- ##
==========================================
+ Coverage 82.11% 82.13% +0.02%
==========================================
Files 1955 1955
Lines 588958 584775 -4183
Branches 44428 44488 +60
==========================================
- Hits 483632 480330 -3302
+ Misses 95677 94794 -883
- Partials 9649 9651 +2
Continue to review full report at Codecov.
|
| the length is equal to the length of the nested list representation of | ||
| the view. The :class:`~memoryview.itemsize` attribute will give you the | ||
| number of bytes in a single element. | ||
| ``len(view)`` is equal to the length of :class:`~memoryview.tolist`, which |
There was a problem hiding this comment.
Since the documented behavior was changed, it needs a versionchanged directive and an entry in What's New.
There was a problem hiding this comment.
Should I put this in the 3.9 what's new? This sounds like a recipe for merge conflicts, until the rest of the patch has general approval.
There was a problem hiding this comment.
Just checking in on this - what's the procedure for adding a what's new entry if I have no idea which python version this will end up being merged into?
There was a problem hiding this comment.
You can use version 3.10. It cannot go into earlier versions since it's a behavior change. And we have ample time for deliberation on how the documented behavior emerged.
There was a problem hiding this comment.
I've update it to say 3.11, since it's been two years since I changed it to say 3.10. Let me know if it should be 3.12 instead.
| @@ -0,0 +1,8 @@ | |||
| 0d ``memoryview`` objects no longer have a ``len``:: | |||
There was a problem hiding this comment.
AFAIK there are issues with multi-paragraph NEWS entry.
There was a problem hiding this comment.
Would you prefer me to keep just the first line here?
There was a problem hiding this comment.
I am not sure that the meaning of words "have a len()" is clear.
Maybe something like "len() for 0-dimensional memoryview objects raises now a TypeError"?
There was a problem hiding this comment.
I've changed to your proposed wording.
|
All commit authors signed the Contributor License Agreement. |
34d362d to
dbad24a
Compare
| ``len()`` for 0-dimensional ``memoryview`` objects (such as ``memoryview(ctypes.c_uint8(42))``) now raises a ``TypeError``. | ||
| Previously this returned ``1``, which was not consistent with ``mem_0d[0]`` raising an ``IndexError``. |
There was a problem hiding this comment.
Presumably there should be a :class: or :type: or :exc: in here somewhere; can a reviewer advise what the convention is?
|
@mdickinson: any chance you're willing to review one more buffer-related PR? This one isn't quite as old as the other two of mine you rescued, but it's getting close! |
mdickinson
left a comment
There was a problem hiding this comment.
The change LGTM in principle, and code looks fine, too.
Question: did you consider using ValueError instead of TypeError? Given that len works for most memoryview objects, the failure is arguably due to value rather than to type. It doesn't seem clear cut: I know NumPy raises TypeError for len(np.array(some_scalar)), and it would be annoying to be inconsistent with that.
I'll leave this open for a few days in case anyone else previously involved in the discussion wants to chip in, and then merge. (@eric-wieser Feel free to ping me if this remains unmerged by April 22nd. The beta 1 cutoff is May 8th.)
|
I'm afraid I don't remember what I considered at the time (which is obviously a lesson in writing better commit messages). I'll see if I can come up with an argument either way. |
|
I suspect consistency with numpy was the main argument; along with consistency with >>> import numpy as np
>>> arr_0d = np.array(42)
>>> mem_0d = memoryview(arr_0d)
>>> len(arr_0d)
TypeError: len() of unsized object
>>> len(mem_0d.tolist())
TypeError: object of type 'int' has no len()
>>> len(mem_0d)
TypeError: 0-dim memory has no length # with this PR |
This makes
memoryview.__len__consistent withmemoryview.__getitem__.https://bugs.python.org/issue39610
(#83791)