Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Stop panic on fmt::Display #3062

Merged
merged 1 commit into from
May 10, 2023
Merged

Stop panic on fmt::Display #3062

merged 1 commit into from
May 10, 2023

Conversation

samuelcolvin
Copy link
Contributor

@samuelcolvin samuelcolvin commented Mar 24, 2023

Closes #3060

@adamreichold
Copy link
Member

Any guidance on why write! isn't available, would be great.

I think you need to fully qualify it, e.g. ::std::write! which seems to work here, e.g.

impl<$($generics,)*> ::std::fmt::Display for $name {
    fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>)
            -> ::std::result::Result<(), ::std::fmt::Error>
    {
        let s = self.str().or(::std::result::Result::Err(::std::fmt::Error))?;
        ::std::write!(f, "{}", s.to_string_lossy())
    }
}

@samuelcolvin
Copy link
Contributor Author

thanks, done.

@adamreichold adamreichold force-pushed the unprintable-object branch 3 times, most recently from 16d6761 to 52dca88 Compare May 9, 2023 10:53
@adamreichold adamreichold marked this pull request as ready for review May 9, 2023 10:53
@adamreichold adamreichold force-pushed the unprintable-object branch from 52dca88 to 9295f81 Compare May 9, 2023 10:54
@adamreichold adamreichold force-pushed the unprintable-object branch from 9295f81 to 5aff2cc Compare May 9, 2023 10:55
@adamreichold
Copy link
Member

Sorry for the many force pushes: I took a detour adding a new API

impl Python<'_> {
    pub fn try_with_gil<F, R>(f: F) -> Option<R>
    where
        F: for<'py> FnOnce(Python<'py>) -> R,
    {
        gil::try_ensure_gil().map(|gil| f(unsafe { gil.python() }))
    }
}

because I did not realise that I already had implicit access to the GIL in the Display impl. Properly removing this and fixing up the news fragment took a bit of back and forth.

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.

This looks good to me, thanks!

@adamreichold
Copy link
Member

bors r=davidhewitt

@bors
Copy link
Contributor

bors bot commented May 10, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 4a0bea2 into PyO3:main May 10, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

py_any.to_string() should not panic
3 participants