removes implicit default of trailing optional arguments (see #2935) #4729
Conversation
Icxolu
force-pushed
the
trailing-options
branch
3 times, most recently
from
635bffa to
9c728f5
Compare
There was a problem hiding this comment.
Thanks for driving this forward, looks good!
I wonder, could we get away without the hard error and just start with the new behaviour immediately? I guess it increases the chances users get silent behaviour change if they didn't notice the deprecation, so maybe it's safer to have it for 1-2 releases, even if a bit verbose... 🤔
We probably can. I initially thought we would do 1 cycle of deprecation and then 1 cycle hard error. Now that we had 2 cycles with deprecation already, it's probably fine to just switch. (Also considering that it's not really super easy to suppress the deprecation. So the vast majority of users should have reacted already.) If you're fine with that, I'll update this and remove the error case. |
|
Yeah, I think it's probably better to skip the hard error phase. If users update more than 2 PyO3 versions in a single hop they are walking into breaking changes without deprecations anyway. 👍 |
Icxolu
force-pushed
the
trailing-options
branch
from
9c728f5 to
a1ad577
Compare
Icxolu
changed the title
Icxolu
force-pushed
the
trailing-options
branch
from
a1ad577 to
d9b2abd
Compare
| python_signature.required_positional_parameters == python_signature.positional_parameters.len(), | ||
| ty.span() => "required arguments after an `Option<_>` argument are ambiguous\n\ | ||
| = help: add a `#[pyo3(signature)]` annotation on this function to unambiguously specify the default values for all optional parameters" | ||
| assert_eq!( |
There was a problem hiding this comment.
Seems reasonable to leave this as an assert 👍
| || quote!(::std::option::Option::None), | ||
| |tokens| some_wrap(tokens, ctx), | ||
| )); | ||
| default = default.map(|tokens| some_wrap(tokens, ctx)); |
There was a problem hiding this comment.
Now that the presence of optionality doesn't potentially insert a default, I hope that one day as a follow up we might be able to move the logic for adding some wrapping from the macro into the generated code / type system.
Would be quite the magical code but it'd be more robust than having to parse Option as a name.
This continues #2935 and
makes trailing optional arguments a hard error (for this release)removes implicit default of trailing optional arguments. This also already makesOption<T>args required, removing the ambiguity for this case already:Closes #2935