GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-114894: add array.array.clear() method by mikeziminio · Pull Request #114919 · python/cpython

Conversation

@mikeziminio

@mikeziminio mikeziminio commented Feb 2, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Issue: Add array.clear() #114894

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mikeziminio
pythongh-114894: add array.array.clear() method
be9733a
@bedevere-app bedevere-app Bot added the awaiting review label Feb 2, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Feb 2, 2024
Add array.clear() #114894
Closed
@blurb-it
📜🤖 Added by blurb_it.
9c2c360
JelleZijlstra
JelleZijlstra reviewed Feb 2, 2024
View reviewed changes
Comment thread Modules/arraymodule.c
return (PyObject *)np;
}

/*[clinic input]

@JelleZijlstra JelleZijlstra Feb 2, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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 in test_array.py but I haven't checked) and a documentation entry (in array.rst)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mikeziminio mikeziminio Feb 2, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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 in test_array.py but I haven't checked) and a documentation entry (in array.rst)

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mikeziminio mikeziminio Feb 6, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mikeziminio
mikeziminio force-pushed the fix-issue-114894 branch from 3368a6b to 9c2c360 Compare February 2, 2024 18:51
mikeziminio added 2 commits February 2, 2024 22:54
@mikeziminio
added test and documentation
8adac42
@mikeziminio
fixed - trim trailing whitespace
e04c2fa
@mikeziminio

mikeziminio commented Feb 2, 2024

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JelleZijlstra
JelleZijlstra self-requested a review February 2, 2024 19:28
JelleZijlstra
JelleZijlstra reviewed Feb 3, 2024
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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))

@JelleZijlstra JelleZijlstra Feb 3, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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 that len(a) == 0?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mikeziminio mikeziminio Feb 6, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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 the test_array.py file works so that the methods are tested for all possible typecode.

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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]))

@JelleZijlstra JelleZijlstra Feb 3, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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()

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mikeziminio mikeziminio Feb 6, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Library/2024-02-02-15-50-13.gh-issue-114894.DF-dSd.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
aisk
aisk reviewed Feb 3, 2024
View reviewed changes
Comment thread Modules/arraymodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@aisk

aisk commented Feb 3, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

In my personal view, according to the original issue, the intention behind adding the clear method is to allow array.array to implement collections.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 in 28bb296

def test_MutableSequence(self):
, so what we need to do is simply include array.array in this existing test case.
sobolevn and mikeziminio reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mikeziminio and others added 4 commits February 6, 2024 09:32
@mikeziminio @aisk
Update Modules/arraymodule.c
7630069 
Co-authored-by: AN Long <[email protected]>
@mikeziminio @JelleZijlstra
Update Misc/NEWS.d/next/Library/2024-02-02-15-50-13.gh-issue-114894.D…
556bf1a 
…F-dSd.rst

Co-authored-by: Jelle Zijlstra <[email protected]>
@mikeziminio
add memoryview test, update other tests
b5b9e63
@mikeziminio
add MutableSequence test
83422b0
@mikeziminio
mikeziminio requested a review from rhettinger as a code owner February 6, 2024 12:11
mikeziminio added 2 commits February 6, 2024 16:14
@mikeziminio
resolve clinic conflicts
4d5fcfb
@mikeziminio
add whatsnew doc
519ad21
@mikeziminio

mikeziminio commented Feb 6, 2024

Copy link
Copy Markdown
Contributor Author

In my personal view, according to the original issue, the intention behind adding the clear method is to allow array.array to implement collections.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 in 28bb296

def test_MutableSequence(self):

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

done

I added only the issubclass check (without isinstance), because for isinstance it would have to be check all typecodes.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mikeziminio
mikeziminio requested review from JelleZijlstra and aisk February 6, 2024 13:28
@mikeziminio
change issue reference
6242242
aisk
aisk reviewed Feb 6, 2024
View reviewed changes
Comment thread Lib/test/test_collections.py Outdated
from random import choice, randrange
from itertools import product, chain, combinations
import string
import array

@aisk aisk Feb 6, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mikeziminio mikeziminio Feb 6, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mikeziminio
move so that it matches the alphabetical order.
fc76d73
@rhettinger
rhettinger removed their request for review February 6, 2024 16:16
JelleZijlstra
JelleZijlstra reviewed Feb 7, 2024
View reviewed changes
Comment thread Doc/whatsnew/3.13.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/library/array.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
mikeziminio and others added 2 commits February 7, 2024 07:19
@mikeziminio @JelleZijlstra
Update Doc/whatsnew/3.13.rst
7b46716 
Co-authored-by: Jelle Zijlstra <[email protected]>
@mikeziminio @JelleZijlstra
Update Doc/library/array.rst
a399ac6 
Co-authored-by: Jelle Zijlstra <[email protected]>
@mikeziminio

mikeziminio commented Feb 8, 2024

Copy link
Copy Markdown
Contributor Author

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aisk

aisk commented Feb 8, 2024

Copy link
Copy Markdown
Member

LGTM, I think the next step is to wait for a core member to approve and merge it.
mikeziminio reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Jason-Y-Z

Jason-Y-Z commented Feb 10, 2024

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JelleZijlstra
JelleZijlstra approved these changes Feb 10, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Feb 10, 2024
@JelleZijlstra
JelleZijlstra merged commit 9d1a353 into python:main Feb 10, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 10, 2024
@Jason-Y-Z

Jason-Y-Z commented Feb 10, 2024

Copy link
Copy Markdown
Contributor

Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
@mikeziminio @blurb-it
@aisk @JelleZijlstra @fsc-eriker
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 <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aisk aisk aisk left review comments
@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@mikeziminio @aisk @Jason-Y-Z @JelleZijlstra
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.