milenkovicm commented on issue #19664: URL: https://github.com/apache/datafusion/issues/19664#issuecomment-3718049470
Thanks @adriangb, I do not see problem with constructors with multiple "required" parameters, they force users to provide all required input. In this specific case, two parameters were required always and two just in case of round-robin, so user has to read the docs or source code to understand when/what to provide (which proposal with two constructors make obvious). In current proposal once user select which partitioner is needed constructor will force input of all required parameters. We could go with the builders in this case, and builders would "start" with same methods as constructors (to enforce required parameters), and there would be no `with_` methods at the moment. I would argue that builders are overkill at the moment for few reasons, its unlikely that this signatures will ever change, with current proposal we saved one `.build()` call, and we did not introduce two builder structs ... Personally I'm not fun of last part of your proposal, you have traded type safety (compiler) check for runtime check, assert in your example would need to be documented. I personally believe apis should enforce constraints on type(s) rather than documentation if/when possible . I do agree that builders are way to go with "optional" parameters, and use of builder in #19619 does make sense (to me). `FilterExecBuilder::new(` which forces you to provide required inputs and `with_` methods which are providing optionals. `build` method does the input validation, as type enforcing check can't be done in that case, so good example of builder. -- 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]
