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

Add support for leading whitespace #433

Open
mxndtaylor opened this issue May 28, 2022 · 3 comments · Fixed by #434
Open

Add support for leading whitespace #433

mxndtaylor opened this issue May 28, 2022 · 3 comments · Fixed by #434

Comments

@mxndtaylor
Copy link
Contributor

Saw this FIXME comment, figured I could help.

@mxndtaylor
Copy link
Contributor Author

Since the method essentially accepts a byte array, I figured is_ascii_whitespace() is good enough?

If it's non-ascii, it'd be considerably more effort to remove in the general case, but the specific cases are easy. So it makes sense to me that responsibility for this should fall on the end user.

@mxndtaylor
Copy link
Contributor Author

Added a PR mostly to demonstrate what I mean. It's still needs some tests and checks run.

Another reasonable addition to it could be to add a warning or an error that makes it more clear to the end user that non-ascii support is somewhat limited. If serde doesn't supports it very well either, then I don't think this is necessary.

@tiagolobocastro
Copy link
Collaborator

I think checking for ascii whitespace only is probably fine for now, as it's better than not doing anything so I've merged it.
For even more improvements, we could also check how serde_json handles non-ascii whitespace?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants