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

migrate IntoPyCallbackOutput to use IntoPyObject #4607

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Oct 9, 2024

This migrates IntoPyCallbackOutput blanket to use IntoPyObject.

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!

Comment on lines 1 to 2
- `IntoPyCallbackOutput` switched blanket implementation to `IntoPyObject`
- `IntoPyCallbackOutput` gained `'py` lifetime parameter
Copy link
Member

Choose a reason for hiding this comment

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

IntoPyCallbackOutput is hidden, so I don't think this needs a changelog..unless it enables users to do things they couldn't do before (does it?). In which case we should document that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't notice that the whole module was hidden (maybe it should be moved into impl_). This should not enable any new things. The trait is used however as a public bound in PyModuleMethods::add_wrapped and PyCFunction constructors. Because the blanket impl changed there could be breakage for downstream types that do not (yet) implement IntoPyObject. The additional lifetime is less of a concern in this case I guess.

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd suggest the following:

  • Just skip the CHANGELOG from this PR
  • Two follow ups (ideally to complete before 0.23), let's open them as issues:
    • Let's move pyo3::callback module into the impl_ submodule
    • Let's move the function constructors to a new trait. They don't need all the generic complexity of IntoPyCallbackOutput, it would be probably good enough to have a simple trait which is implemented for both T: IntoPyObject and Result<T, E> where T: IntoPyObject, E: Into<PyErr>

Comment on lines 1 to 2
- `IntoPyCallbackOutput` switched blanket implementation to `IntoPyObject`
- `IntoPyCallbackOutput` gained `'py` lifetime parameter
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd suggest the following:

  • Just skip the CHANGELOG from this PR
  • Two follow ups (ideally to complete before 0.23), let's open them as issues:
    • Let's move pyo3::callback module into the impl_ submodule
    • Let's move the function constructors to a new trait. They don't need all the generic complexity of IntoPyCallbackOutput, it would be probably good enough to have a simple trait which is implemented for both T: IntoPyObject and Result<T, E> where T: IntoPyObject, E: Into<PyErr>

@Icxolu Icxolu added the CI-skip-changelog Skip checking changelog entry label Oct 10, 2024
@Icxolu Icxolu force-pushed the intopy-callback-output branch from 1014423 to 73ec2dc Compare October 10, 2024 16:54
@Icxolu Icxolu enabled auto-merge October 10, 2024 16:55
@Icxolu Icxolu added this pull request to the merge queue Oct 10, 2024
Merged via the queue into PyO3:main with commit 50a254d Oct 10, 2024
56 of 79 checks passed
@Icxolu Icxolu deleted the intopy-callback-output branch October 10, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants