Skip to content
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

Jump to bottom

gh-94808: Add coverage for bytesarray_setitem #95802

Merged
merged 1 commit into from Oct 10, 2022
Merged

gh-94808: Add coverage for bytesarray_setitem #95802

merged 1 commit into from Oct 10, 2022

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Aug 8, 2022 •
edited by bedevere-bot

When both are provided, tp_ass_subscript takes precedence over tp_ass_item. Since bytesarray provides both, the existing test_setitem tests for bytesarray were not testing bytesarray_setitem, but bytesarray_ass_subscript. This is mostly fine, since Python code has to jump through some hoops to even call it, but a third-party library using PySequence_SetItem could potentially run into this uncovered case.

@mdboom mdboom requested a review from brandtbucher Aug 8, 2022
@mdboom mdboom mentioned this pull request Aug 8, 2022
Open
225 tasks
@mdboom mdboom added the tests Tests in the Lib/test dir label Aug 11, 2022
@mdboom mdboom closed this Aug 12, 2022
@mdboom mdboom reopened this Aug 12, 2022
@mdboom mdboom force-pushed the coverage-bytesarray-setitem branch from 5f24775 to e717c5c Compare Aug 12, 2022
@mdboom mdboom force-pushed the coverage-bytesarray-setitem branch from e717c5c to 1c2a344 Compare Oct 3, 2022
JelleZijlstra
JelleZijlstra approved these changes Oct 7, 2022
View changes
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Looks good. Any reason you didn't mark this for backporting to 3.10?

@JelleZijlstra JelleZijlstra self-assigned this Oct 7, 2022
@mdboom
Copy link
Contributor Author

mdboom commented Oct 10, 2022

Looks good. Any reason you didn't mark this for backporting to 3.10?

No real reason not to -- for all these other coverage improvements linked to #94808, we've just been backporting to 3.11.

@JelleZijlstra JelleZijlstra merged commit dfcdee4 into python:main Oct 10, 2022
12 checks passed
@miss-islington
Copy link
Contributor

miss-islington commented Oct 10, 2022

Thanks @mdboom for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

miss-islington commented Oct 10, 2022

Sorry, @mdboom and @JelleZijlstra, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker dfcdee4a18ece6f7c4fe1aa5830ee861f8b15b24 3.11

@miss-islington
Copy link
Contributor

miss-islington commented Oct 10, 2022

Sorry @mdboom and @JelleZijlstra, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker dfcdee4a18ece6f7c4fe1aa5830ee861f8b15b24 3.10

@JelleZijlstra
Copy link
Member

JelleZijlstra commented Oct 10, 2022

Thanks! I think it's good to backport new tests to the bugfix branches so we can be more confident in any future bugfixes. But testing on 3.11 is definitely more important.

Would you mind doing the manual backports? Feel free to skip 3.10 if you don't think it's worth it.

mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
@mdboom @mpage
pythongh-94808: Add coverage for bytesarray_setitem (python#95802)
fbfd13b
@mdboom mdboom deleted the coverage-bytesarray-setitem branch Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra

@brandtbucher brandtbucher

Assignees

@JelleZijlstra JelleZijlstra

Labels
needs backport to 3.10 needs backport to 3.11 skip news tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mdboom @miss-islington @JelleZijlstra @bedevere-bot