-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Return a value from find_format_args instead of using a callback #11444
Return a value from find_format_args instead of using a callback #11444
Conversation
I haven't reviewed the PR yet. But I don't like the doc comment of
I'll have to look more closely on this PR though, before making a decision. |
Assuming I've done it correctly it should be less of a crime than |
There's nothing that actually stops a Neither of these are really a problem in practice, but there isn't a lifetime you can extend to that isn't technically unsafe. |
9e68abe
to
d3cb51c
Compare
Yeah that's true, I've updated the comment a bit |
☔ The latest upstream changes (presumably #11473) made this pull request unmergeable. Please resolve the merge conflicts. |
d3cb51c
to
ca978aa
Compare
ca978aa
to
c29de92
Compare
I thought about this again today and realised we could stick the |
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.
Wow, turns out if you wait long enough, PR reviews resolve themselves. Sorry for taking so long.
Since this PR doesn't add unsafe code now, it was an easy review.
Thanks!
@bors r+ |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
r? @flip1995
changelog: none