`ValidationErrors` shouldn't use `&'static str`
Opened this issue · 4 comments
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.
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
So does that put the kibosh on this issue then (at least, for now)?
For a bit yes, although the sooner we have a consensus on how errors should look like, the faster there will be a release
Fully agree with @Kyllingene 🥳