Nonstandard StrConsumer trait

[rkuklik]

Hello. I was looking at base64::write::EncoderStringWriter, which requires StrConsumer. However, it is only implemented for String while looking basically as core::fmt::Write.

Could I either provide blanket impl<W: Write> StrConsumer for W (which shouldn't be breaking) or rip it out and switch to Write trait directly (breaking)?

[rkuklik]

On second thought, both options would be breaking, but base64 isn't at vesion 1, so I would prefer trait replacement.

[marshallpierce]

The two traits do bear a superficial similarity, but they do have a different intent to communicate for the user: StrConsumer is just for base64, whereas fmt::Write is more general purpose (any Unicode), so I don't think it should just use fmt::Write directly. However, I agree that it should be possible for a user to use a fmt::Write as a StrConsumer.

I doubt a breaking change in this specific case is likely to affect much (any) calling code, so I'm not very concerned about that aspect, but I still prefer an explicit user choice for the purposes of confirming the user's intent to avoid accidentally using a type in an unexpected way. This could be done by making an adapter struct WriteConsumer<W> (or whatever). Then, if you want to use a Write as a StrConsumer, it's just a WriteConsumer::new(the_write) away.

[marshallpierce]

On the other hand, because there is only really one way that a Write could implement StrConsumer, maybe it is better to just do the blanket impl. Hmm... Competing priorities is what makes API design interesting.

[rkuklik]

The two traits do bear a superficial similarity, but they do have a different intent to communicate for the user: StrConsumer is just for base64, whereas fmt::Write is more general purpose (any Unicode), so I don't think it should just use fmt::Write directly. However, I agree that it should be possible for a user to use a fmt::Write as a StrConsumer.

I don't really see the difference in intent. Both represent types which can consume UTF-8 encoded bytes, while StrConsume doesn't offer any new capabilities or requirements (like Eq and PartialEq have). It is also implemented for String anyway, so I don't see any reason not to switch to Write. It just prevents me from using other types which tend to implement Write, but for which I can't provide trait impl.

I doubt a breaking change in this specific case is likely to affect much (any) calling code, so I'm not very concerned about that aspect, but I still prefer an explicit user choice for the purposes of confirming the user's intent to avoid accidentally using a type in an unexpected way. This could be done by making an adapter struct WriteConsumer<W> (or whatever). Then, if you want to use a Write as a StrConsumer, it's just a WriteConsumer::new(the_write) away.

I am unsure how this could be misused. I expect base64 to be valid string anyway. Before I discovered that this is even a thing, I created something similar here a went to see if there is an optimised version.

[rkuklik]

On the other hand, because there is only really one way that a Write could implement StrConsumer, maybe it is better to just do the blanket impl. Hmm... Competing priorities is what makes API design interesting.

To be honest, I would be for merging these two write Encoders into one, dropped the io::Write bound and added generic impls for io::Write and fmt::Write. But that is my preference, so no big deal.

Also, can base64 not be valid UTF-8? I saw the docs and wondered, why the perf hit? Because if it is, there is no need to use checked from_utf8.

[marshallpierce]

This library doesn't use unsafe, so we have to pay the cost. Fortunately the validation is pretty fast.

[marshallpierce]

To be honest, I would be for merging these two write Encoders into one

They serve different goals. One produces bytes, the other produces str/String.

[marshallpierce]

Unfortunately fmt::Write::write_str() returns Result, but StrConsumer::consume() is infallible, so there's not a safe way adapt a Write to a StrConsumer.

Can you explain more details about your use case? Maybe there's another way to provide an ergonomic solution.

[rkuklik]

This library doesn't use unsafe, so we have to pay the cost. Fortunately the validation is pretty fast.

It doesn't produce strings internally? If not, then there is no reason to add unsafe.

[rkuklik]

Unfortunately fmt::Write::write_str() returns Result, but StrConsumer::consume() is infallible, so there's not a safe way adapt a Write to a StrConsumer.

Can you explain more details about your use case? Maybe there's another way to provide an ergonomic solution.

The docs considered string formatting as infallible, apart from Formatter returning it, but makes sense if it seems bad to use it. As for the ergonomic solution, should I open a PR with my preferred API implemented, which could be discarded if it doesn't feel right?

[rkuklik]

To be honest, I would be for merging these two write Encoders into one

They serve different goals. One produces bytes, the other produces str/String.

I still don't get it, string is just UTF-8 encoded bytes, which I would expect is what base64 yields. So for me the difference is whether the consumer cares about that or not.

[marshallpierce]

As for the ergonomic solution, should I open a PR with my preferred API implemented, which could be discarded if it doesn't feel right?

Sure, go ahead, thanks.

[marshallpierce]

The docs considered string formatting as infallible, apart from Formatter returning it, but makes sense if it seems bad to use it.

🤔 Perhaps we could provide an adapter struct that makes it clear that it will panic on fmt::Error, so that while it's not automatic, it's at least not something users have to write themselves.

[rkuklik]

So I sent my first try, hope it makes sense.

[rkuklik]

Hello, could you please review my changes?

[marshallpierce]

I'll get to it when I can, just been busy.