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

Fix support of sequence protocol for returned lists #730

Merged
merged 2 commits into from
Nov 9, 2022

Conversation

tuxu
Copy link
Contributor

@tuxu tuxu commented Nov 7, 2022

Add the sequence option to PyO3's pyclass, so that the sq_length slot is implemented 1. Implementing this method is required for sequence types, or Python C API functions like PySequence_Size will fail with an error.

The reversed built-in method relies on PySequence_* methods. A test reversing NodeIndices is added to guard against future violations of the sequence protocol.

Fixes #696.

Add the `sequence` option to PyO3's `pyclass`, so that the `sq_length`
slot is implemented [1]. Implementing this method is required for
sequence types, or Python C API functions like `PySequence_Size` will
fail with an error.

The `reversed` built-in method relies on `PySequence_*` methods. A test
reversing `NodeIndices` is added to guard against future violations of
the sequence protocol.

Fixes Qiskit#696.

[1]: PyO3/pyo3#2567
@CLAassistant
Copy link

CLAassistant commented Nov 7, 2022

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@mtreinish mtreinish self-assigned this Nov 7, 2022
@mtreinish mtreinish added the stable-backport-potential This PR or issue is potentially worth backporting for inclusion in a stable branch label Nov 7, 2022
@coveralls
Copy link

coveralls commented Nov 7, 2022

Pull Request Test Coverage Report for Build 3423049318

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 96.947%

Totals Coverage Status
Change from base Build 3396584265: -0.02%
Covered Lines: 13464
Relevant Lines: 13888

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing this! I just pushed up a quick release note to document the fix. Lets merge this, backport it, and we can prepare a 0.12.1 release to get this fix out

@mtreinish mtreinish added the automerge Queue a approved PR for merging label Nov 8, 2022
@tuxu
Copy link
Contributor Author

tuxu commented Nov 8, 2022

LGTM, thanks for fixing this! I just pushed up a quick release note to document the fix. Lets merge this, backport it, and we can prepare a 0.12.1 release to get this fix out

Sure, you're welcome! The sequence option was introduced with pyo3 0.17, so backporting this to 0.12 would require a bump of this dependency. Not sure if that's the right thing to do for a patch-level release.

@mergify mergify bot merged commit 4592bc0 into Qiskit:main Nov 9, 2022
mergify bot pushed a commit that referenced this pull request Nov 9, 2022
* Fix support of sequence protocol for returned lists

Add the `sequence` option to PyO3's `pyclass`, so that the `sq_length`
slot is implemented [1]. Implementing this method is required for
sequence types, or Python C API functions like `PySequence_Size` will
fail with an error.

The `reversed` built-in method relies on `PySequence_*` methods. A test
reversing `NodeIndices` is added to guard against future violations of
the sequence protocol.

Fixes #696.

[1]: PyO3/pyo3#2567

* Add release note

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
(cherry picked from commit 4592bc0)
mergify bot added a commit that referenced this pull request Nov 9, 2022
…735)

* Bump pyo3 from 0.17.2 to 0.17.3 (#723)

Bumps [pyo3](https://github.com/pyo3/pyo3) from 0.17.2 to 0.17.3.
- [Release notes](https://github.com/pyo3/pyo3/releases)
- [Changelog](https://github.com/PyO3/pyo3/blob/main/CHANGELOG.md)
- [Commits](PyO3/pyo3@v0.17.2...v0.17.3)

---
updated-dependencies:
- dependency-name: pyo3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
(cherry picked from commit d91f488)

# Conflicts:
#	Cargo.lock
#	Cargo.toml

* Fix support of sequence protocol for returned lists (#730)

* Fix support of sequence protocol for returned lists

Add the `sequence` option to PyO3's `pyclass`, so that the `sq_length`
slot is implemented [1]. Implementing this method is required for
sequence types, or Python C API functions like `PySequence_Size` will
fail with an error.

The `reversed` built-in method relies on `PySequence_*` methods. A test
reversing `NodeIndices` is added to guard against future violations of
the sequence protocol.

Fixes #696.

[1]: PyO3/pyo3#2567

* Add release note

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
(cherry picked from commit 4592bc0)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Tino Wagner <ich@tinowagner.com>
Co-authored-by: Ivan Carvalho <ivancarv@student.ubc.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Queue a approved PR for merging stable-backport-potential This PR or issue is potentially worth backporting for inclusion in a stable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NodeIndices does not implement sequence protocol
4 participants