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

Derive Deserialize for ValidationErrors #169

Closed
wants to merge 2 commits into from
Closed

Derive Deserialize for ValidationErrors #169

wants to merge 2 commits into from

Conversation

Person-93
Copy link

Needed to change keys to String because the Deserializer can't provide &'static str

Needed to change keys to String because the Deserializer can't provide &'static str
@Keats
Copy link
Owner

Keats commented Sep 6, 2021

What is the usecase there? I'm expecting Validator to only ever serialize things

@Person-93
Copy link
Author

@Keats I need it so that validation can be done on the server and error handling can be done on the client.

@Person-93
Copy link
Author

I'm a bit confused by this error, is it a dependency that doesn't compile on the pinned toolchain?

@Keats
Copy link
Owner

Keats commented Sep 7, 2021

I'm a bit confused by this error, is it a dependency that doesn't compile on the pinned toolchain?

Yes, this specific crate has caused issues across multiple crates :(

This change adds a lot of allocations so I'm not sure it makes sense in that state. Would a Cow<'static> work instead?

@Person-93
Copy link
Author

Yes, it should work.
I was concerned that deserializing a Cow<str> would give it the lifetime of wherever it's derserialized from, but based on this issue it looks like it does not and there are no plans to change that.
I'll fix it.

@krojew
Copy link
Contributor

krojew commented Mar 30, 2022

Can you rebase your PR?

@Person-93
Copy link
Author

Can you rebase your PR?

I've deleted my fork, but I looked around and found this commit which looks like it has all my changes. Feel free to clone it and open your own PR.

@Person-93 Person-93 closed this Mar 30, 2022
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.

3 participants