paperclip icon indicating copy to clipboard operation
paperclip copied to clipboard

Add support for leading whitespace

Open mxndtaylor opened this issue 2 years ago • 3 comments

Saw this FIXME comment, figured I could help.

mxndtaylor avatar May 28 '22 14:05 mxndtaylor

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 avatar May 28 '22 15:05 mxndtaylor

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.

mxndtaylor avatar May 28 '22 15:05 mxndtaylor

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?

tiagolobocastro avatar Jun 04 '22 23:06 tiagolobocastro