gabotechs commented on code in PR #26565:
URL: https://github.com/apache/datafusion/pull/26565#discussion_r4375451398
##########
docs/source/library-user-guide/upgrading/56.0.0.md:
##########
@@ -1185,9 +1182,7 @@ A session carries one registry. Each library adds its
decoders to it with `Sessi
```rust,ignore
// In each library that ships extension types:
pub fn register(config: &mut SessionConfig) -> Result<()> {
- config.update_extension(|registry: &mut ProtoDecoderRegistry| {
- register_execution_plan::<MyExec>(registry)
- })
+ config.update_extension(MyExec::register)
}
Review Comment:
Previously, because of the boilerplate the compiler required you to write,
it was explicit that you were registering a proto codec, however, now there's
no longer any hint about that in the code:
```rust
config.update_extension(MyExec::register)
```
People unfamiliar with this might read the snippet above and would not know
what's exactly getting registered. Maybe this can be alleviated by a simple
method rename to something like `MyExec::register_proto`.
My impression is that, even if I see the ergonomics point, it's very subtle
and I'd personally not double index too much on improving it, it's already
great with your latest PRs.
That being said, I don't feel strongly about it, so if you want to push it
forward then +1 for me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
