-
Notifications
You must be signed in to change notification settings - Fork 856
Draft: Rename pyo3::prepare_freethreaded_python
#5247
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for the contribution! Other than needing a newsfragment, what makes this WIP? I'd love to merge for the upcoming release. |
@davidhewitt Some of the tests were failing for me locally on the |
@davidhewitt
The tests are now passing in the However, the tests on the
|
pyo3::prepare_freethreaded_python
to pyo3::initialize_python
pyo3::prepare_freethreaded_python
to pyo3::initialize_python
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.
Did we rule out Python::initialize
? That would sounds intuitive to me and it would be near to the other functions that interact with the runtime like Python::attach
and Python::detach
. Might be nice for discoveribility.
/// Python::attach(|py| py.run(pyo3::ffi::c_str!("print('Hello World')"), None, None)) | ||
/// # } | ||
/// ``` | ||
#[cfg(not(any(PyPy, GraalPy)))] | ||
pub fn prepare_freethreaded_python() { | ||
pub fn initialize_python() { |
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.
I think we should keep the old name with a deprecation added to offer a smoother transition to users.
I agree, it makes sense. I'll need to look into the implementation. |
An issue would be helpful, yes please. |
Nicely done @olp-cs! You may find this helpful: https://rustwiki.org/en/reference/attributes/diagnostics.html#the-deprecated-attribute An example of it in PyO3 can be found here: pyo3/pyo3-ffi/src/intrcheck.rs Line 13 in 7836f15
|
pyo3::prepare_freethreaded_python
to pyo3::initialize_python
pyo3::prepare_freethreaded_python
Thank you! I'll look into this.
I've created it here: #5254 |
@olp-cs just checking to see when you expect to have a chance to return to this? I think the agreement is that Reason why I ask - I'd love to get this into the 0.26 release, which I'd like to publish in the next week or two. I prefer to leave time for contributors to cycle back when they can and fully understand everyone has different availability and resources. In this case this rename really fits being released alongside the rename of If you don't have availability to finish this in the next couple weeks, I totally understand. I can push to this branch and finish this off if you will forgive me 🙏 . |
@davidhewitt Thank you for the explanations! If you don't hear back from me by 30 July, feel free to take it over. Apologies for the delay, I was at another conference right after EuroPython, and I've just came back from Prague. I'll try to get back to this issue within the next couple of days. |
Related to #5061
At EuroPython Sprints with @Cheukting