NotificationsYou must be signed in to change notification settings
Fork32.4k
Star67.9k

gh-114894: add array.array.clear() method#114919

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

Jump to bottom

Merged

JelleZijlstra merged 14 commits intopython:mainfrommikeziminio:fix-issue-114894

Feb 10, 2024

Merged

gh-114894: add array.array.clear() method#114919

JelleZijlstra merged 14 commits intopython:mainfrommikeziminio:fix-issue-114894

Feb 10, 2024

Conversation

Copy link

Contributor

mikeziminio commentedFeb 2, 2024•
edited by bedevere-appbot
Loading

Issue:Add array.clear() #114894

pythongh-114894: add array.array.clear() method

be9733a

bedevere-appbot added the awaiting review label

Feb 2, 2024

bedevere-appbot mentioned this pull request

Feb 2, 2024

Add array.clear()#114894

Closed

📜🤖 Added by blurb_it.

9c2c360

JelleZijlstra reviewed

Feb 2, 2024

View reviewed changes

Modules/arraymodule.c

		@@ -868,6 +868,20 @@ array_slice(arrayobject *a, Py_ssize_t ilow, Py_ssize_t ihigh)
		return (PyObject *)np;
		}

		/*[clinic input]

Copy link

Member

Choose a reason for hiding this comment

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

This will also need new unit tests (probably intest_array.py but I haven't checked) and a documentation entry (inarray.rst)

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

This will also need new unit tests (probably intest_array.py but I haven't checked) and a documentation entry (inarray.rst)

Thanks for the comment. I'll try to do it now.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

done

mikeziminio force-pushed thefix-issue-114894 branch from3368a6b to9c2c360Compare

February 2, 2024 18:51

mikeziminio added2 commits

February 2, 2024 22:54

added test and documentation

8adac42

fixed - trim trailing whitespace

e04c2fa

Copy link

ContributorAuthor

mikeziminio commentedFeb 2, 2024

@JelleZijlstra Jelle, I've added tests and documentation)

JelleZijlstra self-requested a review

February 2, 2024 19:28

JelleZijlstra reviewed

Feb 3, 2024

View reviewed changes

Copy link

Member

JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Can you also add this to the What's New document for Python 3.13 (Doc/whatsnew/3.13.rst)?

Lib/test/test_array.py Outdated

		a = array.array(self.typecode, self.example)
		self.assertRaises(TypeError, a.clear, 42)
		a.clear()
		self.assertEqual(a, array.array(self.typecode))

Copy link

Member

Choose a reason for hiding this comment

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

This is a bit of an indirect test, why not instead test thatlen(a) == 0?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

All the testing logic from thetest_array.py file works so that the methods are tested for all possibletypecode.

To be honest, initially I used array comparison expecting that there would be atypecode check, but then I looked atarray_richcompare()and realized that comparing arrays using the== operator is not suitable for checking the sametypecode.

Therefore, I added two checks - thelen() you suggested and thetypecode comparison (just in case of future optimizations).

Lib/test/test_array.py

		a.append(self.example[2])
		a.append(self.example[3])
		self.assertEqual(a, array.array(self.typecode, self.example[2:4]))

Copy link

Member

JelleZijlstraFeb 3, 2024

Choose a reason for hiding this comment

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

Add a test that an array with exported buffers cannot be cleared, something like this (untested):

with memoryview(a):    with self.assertRaises(BufferError):        a.clear()

Copy link

ContributorAuthor

mikeziminioFeb 6, 2024

Choose a reason for hiding this comment

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

done

Misc/NEWS.d/next/Library/2024-02-02-15-50-13.gh-issue-114894.DF-dSd.rst OutdatedShow resolvedHide resolved

aisk reviewed

Feb 3, 2024

View reviewed changes

Modules/arraymodule.c OutdatedShow resolvedHide resolved

Copy link

Member

aisk commentedFeb 3, 2024•
edited
Loading

In my personal view, according to the original issue, the intention behind adding theclear method is to allowarray.array to implementcollections.abc.MutableSequence, so we can add a test to test it and ensure we don't break it in the future.

There is already a relevant test case in

cpython/Lib/test/test_collections.py

Line 1968 in28bb296

deftest_MutableSequence(self):

, so what we need to do is simply includearray.array in this existing test case.

mikeziminioand others added4 commits

February 6, 2024 09:32

Update Modules/arraymodule.c

7630069

Co-authored-by: AN Long <aisk@users.noreply.github.com>

Update Misc/NEWS.d/next/Library/2024-02-02-15-50-13.gh-issue-114894.D…

556bf1a

…F-dSd.rstCo-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

add memoryview test, update other tests

b5b9e63

add MutableSequence test

83422b0

mikeziminio requested a review fromrhettinger as acode owner

February 6, 2024 12:11

mikeziminio added2 commits

February 6, 2024 16:14

resolve clinic conflicts

4d5fcfb

add whatsnew doc

519ad21

Copy link

ContributorAuthor

mikeziminio commentedFeb 6, 2024

In my personal view, according to the original issue, the intention behind adding theclear method is to allowarray.array to implementcollections.abc.MutableSequence, so we can add a test to test it and ensure we don't break it in the future.
There is already a relevant test case in
cpython/Lib/test/test_collections.py
Line 1968 in28bb296
deftest_MutableSequence(self):
, so what we need to do is simply includearray.array in this existing test case.

done

I added only theissubclass check (withoutisinstance), because forisinstance it would have to be check all typecodes.

mikeziminio requested review fromaisk andJelleZijlstra

February 6, 2024 13:28

change issue reference

6242242

aisk reviewed

Feb 6, 2024

View reviewed changes

Lib/test/test_collections.py Outdated

		@@ -9,6 +9,7 @@
		from random import choice, randrange
		from itertools import product, chain, combinations
		import string
		import array

Copy link

Member

aiskFeb 6, 2024

Choose a reason for hiding this comment

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

Nitpick: These imports are basically sorted alphabetically, so this line may move to the first line.

Copy link

ContributorAuthor

mikeziminioFeb 6, 2024

Choose a reason for hiding this comment

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

done

move so that it matches the alphabetical order.

fc76d73

rhettinger removed their request for review

February 6, 2024 16:16

JelleZijlstra reviewed

Feb 7, 2024

View reviewed changes

Doc/whatsnew/3.13.rst OutdatedShow resolvedHide resolved

Doc/library/array.rstShow resolvedHide resolved

mikeziminioand others added2 commits

February 7, 2024 07:19

Update Doc/whatsnew/3.13.rst

7b46716

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

Update Doc/library/array.rst

a399ac6

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

Copy link

ContributorAuthor

mikeziminio commentedFeb 8, 2024

@JelleZijlstra @aisk Guys, if I understand correctly, I made all the changes according to the reviews.
Please tell me what further steps are needed to merge the PR.

Copy link

Member

aisk commentedFeb 8, 2024

LGTM, I think the next step is to wait for a core member to approve and merge it.

Copy link

Contributor

Jason-Y-Z commentedFeb 10, 2024

Thanks all for the efforts, sorry for tagging but gentle nudge to@vstinner who seems to have touched the relevant files recently (according toBLAME).

JelleZijlstra approved these changes

Feb 10, 2024

View reviewed changes

bedevere-appbot added awaiting merge and removed awaiting review labels

Feb 10, 2024

JelleZijlstra merged commit9d1a353 intopython:main

Feb 10, 2024

bedevere-appbot removed the awaiting merge label

Feb 10, 2024

Copy link

Contributor

Jason-Y-Z commentedFeb 10, 2024

Thanks!

fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request

Feb 14, 2024

pythongh-114894: add array.array.clear() method (python#114919)

fe2a0df

Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>Co-authored-by: AN Long <aisk@users.noreply.github.com>Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

Labels

None yet

4 participants

Movatterモバイル変換

Uh oh!

gh-114894: add array.array.clear() method#114919

gh-114894: add array.array.clear() method#114919

Uh oh!

Conversation

mikeziminio commentedFeb 2, 2024• edited by bedevere-appbotLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeziminio commentedFeb 2, 2024

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aisk commentedFeb 3, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

mikeziminio commentedFeb 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mikeziminio commentedFeb 8, 2024

Uh oh!

aisk commentedFeb 8, 2024

Uh oh!

Jason-Y-Z commentedFeb 10, 2024

Uh oh!

Jason-Y-Z commentedFeb 10, 2024

Uh oh!

Uh oh!

mikeziminio commentedFeb 2, 2024•
edited by bedevere-appbot
Loading

aisk commentedFeb 3, 2024•
edited
Loading