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

Remove built-in line wrapping #60

Closed
marshallpierce opened this issue May 7, 2018 · 7 comments
Closed

Remove built-in line wrapping #60

marshallpierce opened this issue May 7, 2018 · 7 comments

Comments

@marshallpierce
Copy link
Owner

It turns out that line wrapping is a major source of complexity for not a lot of user convenience, and I think we should get rid of it. (We're not alone in this stance on Base64 -- Java and Go stdlib base64 also doesn't support it.)

It makes ChunkedEncoder and the work in #56 much more complex (and similar goals in #20), and I suspect it's not really necessary to support optimized (and complex) intermingling of encoding and line wrapping: are there really people out there who are encoding massive amounts of line-wrapped base64? It seems unlikely.

Anyway, here's what I'm thinking:

  • Remove line wrapping from Config. This would mean all line wrapping logic from various encoding routines could be stripped out, and the complication in ChunkedEncoder could also be removed.
  • Rejoice at the simplification of base64 streaming encoder #56.

Base64Display would no longer be able to do line wrapping, but it already had only partial support (in that really weird line configurations would error). That will be tough for users to replicate unless we expose ChunkedEncoder publicly, but I think the main use case of that was things like HTTP headers, which don't need line wrapping.

For Read/Write wrapper implementations, we can just provide an extra set of wrappers that only do line wrapping, which users can use as they see fit. Or maybe don't even bother with that?

For plain ol' "base64 these bytes with some line breaks", if we exposed the line_wrap module more or less as-is, that's pretty much all that would be needed for, say, MIME.

@zetok
Copy link

zetok commented Aug 19, 2018

(…) are there really people out there who are encoding massive amounts of line-wrapped base64?

Yes, I use only line-wrapped base64 in one of my projects, and it is supposed to deal with huge amounts of data. Actually, right now I'm in process of moving from old rustc_serialize to this base64 crate. No support for line-wrapping would automatically disqualify the crate.

However in my case I'd be pretty satisfied with standard MIME line wrapping, as it is what I've been using, and it's what rustc_serialize provided.

So rather than removing line-wrapping, I'd suggest to just do it the way rustc_serialize did it, which seems to work quite well.

https://docs.rs/rustc-serialize/0.3.24/rustc_serialize/base64/index.html

@marshallpierce
Copy link
Owner Author

Thanks for chiming in with your use case.

In your project, are you looking for base64 + line wrapping that doesn't heap allocate? That's the hard part. If all you need is the ability to take, say, a String, and line wrap it in place while moving a minimum number of bytes around, that's easy enough and would be provided by more or less exposing the internal line wrap module as is.

@marshallpierce
Copy link
Owner Author

Removed in 7fa9f4d.

@P-E-Meunier
Copy link

Oops, that was actually a super useful feature for parsing SSH keys in Thrussh, its removal broke lots of stuff on my side, especially since getting SSH keys right is surprisingly hard.

@marshallpierce
Copy link
Owner Author

Line wrapping is still available; it's just now a separate crate. https://crates.io/crates/line-wrap

@marshallpierce
Copy link
Owner Author

Oh I see, you meant removing whitespace on parsing: that's easy too. Just filter out whitespace ahead of time -- that's all it was doing internally (allocating a new vec to hold the filtered results of iteration).

@marshallpierce
Copy link
Owner Author

Also, ICYMI -- the release notes include the snippet to strip whitespace.

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

No branches or pull requests

3 participants