Keats / validator

Simple validation for Rust structs
MIT License
1.91k stars 140 forks source link

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

Open Kyllingene opened 4 months ago

Kyllingene commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

Keats commented 4 months ago

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 commented 4 months ago

Fully agree with @Kyllingene 🥳