Replace string crate with a handrolled type

[bkirwi]

Addresses: #43

• The first commit adds a couple of TopicName helpers that make it easier to convert to/from normal string types.
• The second commit replaces the entire dep with a from-scratch reimplementation.

The second commit involves a small bit of unsafe code. If that's a problem for us, there are options to remove it (at some possible runtime cost) which we could discuss. But maybe it's fine? I'm personally a bit split on it.

[tychedelia]

Thanks, this is really straightforward and seems like a great improvement. Will you just remove string from our deps?

[tychedelia]

Wondering if we also want to provide unchecked constructors, although perhaps it's best to proceed with the entirely safe API first and see if anyone has a need for them later.

[tychedelia]

Also @bkirwi It looks like your first commit got clobbered by the second. I'm not seeing the topic name changes in the diff.

[rukai]

Thanks a lot, the old string type was causing us all sorts of problems.
I didnt think to replace it!

[bkirwi]

Will you just remove string from our deps?

string should be removed from the deps here: https://github.com/tychedelia/kafka-protocol-rs/pull/44/files#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542L32

I've checked cargo tree and confirmed that it's no longer present in the transitive deps.

It looks like your first commit got clobbered by the second. I'm not seeing the topic name changes in the diff.

Apologies - this was semi-intentional, and I should have called it out! We get the as_str for free from the Deref implementation. We don't have the From<String> for TopicName, though... StrBytes::from_string(s).into() felt lightweight enough that I didn't bother re-introducing it, but I'd be happy to if y'all prefer.

It looks like I have a merge conflict here... I should be able to address it in the next few days, but feel free to push to the branch if you'd rather push this through sooner!

[tychedelia]

string should be removed from the deps here

Ope! Somehow missed that. Apologies.

StrBytes::from_string(s).into() felt lightweight enough that I didn't bother re-introducing it, but I'd be happy to if y'all prefer.

It's not too bad at all, but I definitely think "name".into() would be very ergonomic. If you want to, this can be a follow up (in addition to GroupId which is the other StrBytes type). Thanks again for this contribution!

[thedodd]

🎉 this is great! It has also been a major ergonomics issue for me as well!