Structural error

[andy128k]

My attempt to resolve #60

[TedDriggs]

Thanks for submitting this. It's going to be a couple days before I can test it locally, but once I do I'll be able to provide the final review and will merge if everything looks good.

[andy128k]

@TedDriggs any updates on this?

[TedDriggs]

Sorry, my day job has been taking up a lot of my time and I haven't had a chance to test this. I think the code is all good, and the hangup is purely my "rustiness" with this code base. Here are my outstanding concerns:

1. I don't exactly remember how derive-builder handles the std-vs-nostd behavior, so I think this is correct, but I'm not 100% sure
2. My ad-hoc inspection of builder errors suggests that they're primarily just having unwrap called on them, which leads me to think that this is safe. However, I don't know that I have a good way to test that theory, and even in 0.10.0 I'm loathe to break people's existing code
3. We're using non_exhaustive correctly, but I still feel bad about bumping rustc to 1.45.0 when moving to 0.10.0

Generating an error per builder is a much cleaner solution than trying to make one structured error that the main crate emits. That's definitely the right path forward.

I'm wondering if a good interim path forward is:

• Make using the error type an option passed to #[builder(error(custom_enum))] or something and ship that as 0.9.1. That would give early adopters the chance to use it now
• When we release 0.10.0, make it the default but add a way to access the legacy string behavior

Thoughts?

[andy128k]

@TedDriggs

1. I do not have experience with no_std. It would be good to add automated tests for it.
2. I have an impression that people tend to .unwrap() or to .map_err(). This change doesn't break first use case. Those who map_err() either will welcome this change and tidy their code by removing map_err or will need to add .to_string(). While change is breaking, impact is not that big.
   I, personally decided to work on this issue because now I am one of the maintainers of rss crate whose users are asking for better error types.
3. Let me correct you. it is not 1.45.0 but 1.40.0 which was released 2019-12-19.

0.10.0-alpha/beta versions can be released for early adopters. SemVer helps here. Only those who use version * will be affected. So, potential harm and blast radius are small.

[TedDriggs]

You are right. There used to be no_std tests, but I ended up disabling them because they broke too often. Let's move ahead with this on 0.10.0-alpha and see how we go.

[andy128k]

Conflicts are resolved.

[TedDriggs]

I'm still thinking about the name of the variant for validation errors; I think I might switch it to Custom to follow what serde does. However, this is awesome work and I'm really grateful to you for bringing it in.

[EndilWayfare]

My apologies, it's a little difficult for me to follow the macro shenanigans. It seems like the return type of the validator is now Result<(), Into<#error_ty>> instead of Result<(), String> (and FooBuilder: From<String> just an example implementation detail). Am I reading this correctly?

[EndilWayfare]

Ok, wait, I'm sorry. I think I've got that wrong. error_ty is the enum, and the ValidationError variant just has String data. What if there were a... validation_error_ty that defaults to String? I'd really prefer structured validation errors.