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

Implement ToPyObject for [T; N] #2313

Merged
merged 1 commit into from Apr 19, 2022
Merged

Implement ToPyObject for [T; N] #2313

merged 1 commit into from Apr 19, 2022

Conversation

PigeonF
Copy link
Contributor

@PigeonF PigeonF commented Apr 18, 2022

It seems like this was forgotten to implement (unless there was a good reason not to implement this, which I did not find in my quick search).

Related: fusion-engineering/inline-python#51

@adamreichold
Copy link
Member

CI failure is unrelated and affects main as well ATM.

@mejrs mejrs self-requested a review April 18, 2022 17:43
Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

Thanks!

I'm not sure why this was missing. I suppose in most cases, like when calling the method on an array, it can find the &[T] implementation - but inline-python uses it in a generic context where this does not work.

src/conversions/array.rs Outdated Show resolved Hide resolved
@konstin
Copy link
Member

konstin commented Apr 18, 2022

It seems like this was forgotten to implement (unless there was a good reason not to implement this, which I did not find in my quick search).

impl<T, const N: usize> was only stabilized in rust 1.51, i'd have loved to have done this earlier! i'm surprised though we didn't have any impl for small slice lengths, since for tuples we have the tuple_conversion macro which implements tuples up to size 12.

@messense

This comment was marked as resolved.

@birkenfeld
Copy link
Member

It's already guarded by a feature flag, which is set by build.rs depending on version. rustversion is not used as a non-dev dependency.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Yep I think this was just an accidental omission 👍

@davidhewitt
Copy link
Member

Rebasing on main should fix the 3.11 error

@mejrs mejrs merged commit dea9eb7 into PyO3:main Apr 19, 2022
@mejrs
Copy link
Member

mejrs commented Apr 19, 2022

Thanks!

mejrs pushed a commit to mejrs/pyo3 that referenced this pull request Apr 20, 2022
mejrs pushed a commit to mejrs/pyo3 that referenced this pull request May 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants