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

Restore base64::{encode, decode} functions #213

Closed
rillian opened this issue Jan 9, 2023 · 11 comments
Closed

Restore base64::{encode, decode} functions #213

rillian opened this issue Jan 9, 2023 · 11 comments

Comments

@rillian
Copy link

rillian commented Jan 9, 2023

This is a request to restore the base64::encode and base64::decode convenience functions deprecated in the 0.21.0 release.

Normally I agree with explicit being better than implicit, but I don't see the harm here. If there's no specific interoperability requirements, explicitly choosing a configuration isn't particularly important. For quick scripts, adding all of

// Import the base64 crate Engine trait anonymously so we can
// call its methods without adding to the namespace.
use base64::engine::Engine as _;
use base64::engine::general_purpose::STANDARD as BASE64;

at the top of the file and then calling BASE64.encode() is more cognitive load. With the new API I have to think about the trait, the current namespace, and why I'm calling a method on a constant. That's being explicit about the crate's implementation choices, not just the base64 variant.

In contrast, when I see a bare base64::encode() its clear some base64 implementation is being called to encode something in the default config, which is good enough a lot of the time.

@marshallpierce
Copy link
Owner

Sorry, I have no interest in making that style of coding easier. I want users to consciously choose what config they're using. I view blindly picking a default as a mistake, and I don't want to enable that, much in the same way that Java's concept of a "default text encoding" was also a mistake.

@rillian
Copy link
Author

rillian commented Jan 10, 2023

Thank you for explaining your motivation.

@MathieuDuponchelle
Copy link

Sorry, I have no interest in making that style of coding easier. I want users to consciously choose what config they're using. I view blindly picking a default as a mistake, and I don't want to enable that, much in the same way that Java's concept of a "default text encoding" was also a mistake.

Could we reach a middle ground here and have a more explicit convenience API, eg base64::standard_decode ?

I had never used this crate and had to use it yesterday, and without experience of the previous API I still found the current API quite cumbersome, especially the use base64::engine::Engine as _; part leaves a pretty bad taste :)

@bradfier
Copy link

bradfier commented Jan 11, 2023

Rust has a 'default text encoding' and that doesn't appear to be a justification to put every string method behind a StringLike trait, because that would be masses of cognitive overhead for very little benefit.

A convenience API as @MathieuDuponchelle suggests feels like the best compromise here if you're determined to deprecate encode and decode - at the moment 0.21 simply makes the library more difficult to use which I can't imagine was the intent.

@marshallpierce
Copy link
Owner

The encode() and decode() methods are not coming back. They were a mistake that predated my involvement in the project.

Rust's "default text encoding" is not a valid comparison because it is the only encoding used, and is provided as a guarantee throughout that text is UTF-8. The concept of default encoding in Java is a better comparison, where it has not worked out well.

If things like "using traits" are seen as too much of a burden, I can't help you.

@sdroege
Copy link

sdroege commented Jan 12, 2023

You're absolutely right that it should be an explicit choice which variant of base64 should be used (just like with e.g. CRC32 and percent encoding), but the way the current API is defined is really not great. It requires a lot of boilerplate to do such a simple task. There must be a better way for the boring cases where one wants just one of the standard alphabets.

Even having a set of crate-level functions in the style of fn decode<T: Engine>(input: impl AsRef<[u8]>) -> Result<...> would make usage less cumbersome for the simple case as it wouldn't require bringing the trait into scope.

That's also close to how the percent_encoding (alphabet as parameter) and crc (crate-level constants for the standard algorithms with directly accessible methods) crates work. Having a look at how other, similar crates handle this would seem like a good idea instead of only providing this usage-unfriendly API.

@marshallpierce
Copy link
Owner

This has already been discussed extensively elsewhere. I am not sympathetic to concerns over useing a trait. Rust is not the language for you if you don't like traits. Feel free to collect a full refund on your way out.

@sdroege
Copy link

sdroege commented Jan 12, 2023

Thanks for making clear that you care more about feeling superior than caring about API usability. I'll make sure to not use any of your crates if I can avoid it then. Too bad that one of your crates is occupying such a prominent name on crates.io. I hope you're feeling good about all the beginners that are just learning Rust and need base64 handling that are turned away by "base64 handling in Rust is just too complicated". Should they just go back to JavaScript, right?

And to be clear, this is not about not being able to understand your API. Just that usage of it simply sucks more than it has to. You might want to look at some of my crates if you feel like I'm just too stupid to deal with traits.

@marshallpierce
Copy link
Owner

@sdroege has earned himself a ban. Anyone else who has never contributed an ounce of effort to this library feel like criticizing the work I do for free?

@sdroege
Copy link

sdroege commented Jan 12, 2023

Thanks, likewise!

@marshallpierce
Copy link
Owner

shakes head some people just can't take a hint I guess. Locking this issue.

Repository owner locked as too heated and limited conversation to collaborators Jan 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants