`ValidationErrors` shouldn't use `&'static str`

[Kyllingene]

ValidationErrors::add (and siblings) currently require &'static str. That's because ValidationErrors is a wrapper around HashMap<&'static str, ...>.

I understand the desire to avoid allocations where possible, but this is an unnecessarily restrictive API. Simply changing to String would be better: sure, it forces some users to allocate when they currently do not, but it's better than making certain use-cases impossible.

The change could be hidden by performing the allocation inside add, making it non-breaking. Whether or not that's acceptable is up for debate; the next methods will be breaking.

Alternatively, you could refactor to Cow<'static, str>, and expose that to the user. It could maintain backwards compatibility in function arguments by accepting Into<Cow<'static, str>>, though it would still break compat thanks to errors() and the like.

This does require some refactoring, I'll admit, and can lead to extra allocations in the event that the user does pass in an owned value. However, I've done the refactor (taking the Into<Cow> path) and updated the tests, and everything seems to work.

In any case, this is better than the status quo: users are currently forced to leak their Strings to use them as fields, which is usually a much bigger performance problem than allocating more strings.

[Keats]

The next big breaking change I'm thinking of will be to refactor the errors. I'll create a meta-issue next week for that

[Kyllingene]

So does that put the kibosh on this issue then (at least, for now)?

[Keats]

For a bit yes, although the sooner we have a consensus on how errors should look like, the faster there will be a release

[carlocorradini]

Fully agree with @Kyllingene 🥳

[DenuxPlays]

Any progress?

Currently it is impossible to translate errors or map them from other error types.

[Keats]

I don't have time right now to work on it. I took a quick stab at it a few weeks ago but didn't get very far.
I think the code above https://github.com/Keats/validator/blob/master/validator/src/types.rs#L50 makes sense. Then we make L50 use Cow like the rest. I find the functions in the impl below pretty confusing though.

it would be nice to have some user reports on the whole ValidationErrors usage. Is it working fine outside of that lifetime? Too confusing? Hard to use in custom functions?

[DenuxPlays]

For me yeah its working fine except for the lifetimes.
But I dont think Cow is the solution because it also requires 'static.

[Keats]

What lifetime would you want?

[DenuxPlays]

Something non static.

best example is the translations again.
It is almost impossible to include all translations in your binary and if you then want to add the possibility for the user to add their own translation its game over.

Whats wrong with a normal &str or String?

[Keats]

Translations is not really meant to be done on validator directly: each error can have a code and then you can get the right error message based on that code somewhere else (be it gettext or fluent or whatever).

Whats wrong with a normal &str or String?

&str wouldn't work, you would need to specify a lifetime.
String would allocate for something that is static 99% of the time. Cow<'static, str> should work if you want to use String

[DenuxPlays]

Yeah you're right.
But I am not a huge fan to have limitations that aren't really necessary.
But maybe that's just my opinion and others are fine with this.

[Keats]

Fixed in 0.20