Skip to content
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

Only generate ValidationError when needed #203

Closed
wants to merge 3 commits into from

Conversation

TedDriggs
Copy link
Collaborator

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

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
Copy link
Collaborator Author

@andy128k please take a look

rust-analyzer didn't touch trailing whitespace in a macro,
which caused rustfmt to bail out.
@andy128k
Copy link
Contributor

andy128k commented Apr 3, 2021

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
Copy link
Collaborator Author

The example now calls that out with the phantom_validate function.

@andy128k
Copy link
Contributor

andy128k commented Apr 3, 2021

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

@TedDriggs
Copy link
Collaborator Author

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
Copy link
Contributor

andy128k commented Apr 6, 2021

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

@TedDriggs
Copy link
Collaborator Author

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
Copy link
Contributor

andy128k commented Apr 7, 2021

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
Copy link
Collaborator Author

Went with #204 instead

@TedDriggs TedDriggs closed this Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't compile v0.10.0 in the no_std environment
2 participants