colin-kiegel / rust-derive-builder

derive builder implementation for rust structs
https://colin-kiegel.github.io/rust-derive-builder/
Apache License 2.0
1.32k stars 88 forks source link

Only generate ValidationError when needed #203

Closed TedDriggs closed 3 years ago

TedDriggs commented 3 years ago

Custom validation errors are converted to strings if the deriving struct doesn't specify a type for the errors returned by the build method. This variant was generated even when it wasn't needed, resulting in a reference to String that broke no_std.

This change only emits that variant when build_fn(validate = "...") is set and build_fn(error = "...") is not set. This means no_std will work in the simple case with no additional meta items needed.

Fixes #201

TedDriggs commented 3 years ago

@andy128k please take a look

andy128k commented 3 years ago

This may break a compatibility with a code where custom validation is not provided but a field is annotated as #[builder(default = "self.default_field()?")] and default_field() returns Result<_, String>.

TedDriggs commented 3 years ago

The example now calls that out with the phantom_validate function.

andy128k commented 3 years ago

Another option is to not generate ValidationError at all and advice users to do #[builder(build_fn(error = "Box<dyn std::error::Error>"))].

TedDriggs commented 3 years ago

I have a preference for continuing to generate the variant in the case that a validation function is provided, as that avoids making the use of validate force a change right away. Also, I don't think String implements std::error::Error, so that wouldn't work for anyone who had been using String before.

andy128k commented 3 years ago

String does not implement an Error trait but still works well with Box<dyn Error>.

TedDriggs commented 3 years ago

Today I learned. I’m still inclined to preserve the 0.10.0 decision of using String for the error type in the generated case and omitting it if we don’t need the variant. I don’t love that it’s “magical” but I really don’t want to break people using validate more

andy128k commented 3 years ago

Version 0.9.0 emitted build method like fn build(self) -> Result<T, ::alloc::string::String> and this was acceptable in no_std environment. So, probably ValidationError could use alloc::string::String too?

TedDriggs commented 3 years ago

Went with #204 instead