The DeviceId type alias is hard to use.

[poljar]

I feel like using DeviceId isn't as nice as it used to be now that DeviceId is a type alias over str instead of String. I think we only had a couple of friction points in the matrix-rust-sdk where we wanted to use &DeviceId, now we have plenty of places where we need to box the device id.

I know this isn't really important for the matrix-rust-sdk public API since we can hide it, but investing some time into a wrapper type for this might make Ruma nicer in general for everyone.

Perhaps wrapping a Cow might work, or the unicase case (🥁) mentioned over here sounds nice as well.

[jplatte]

investing some time into a wrapper type for this might make Ruma nicer in general for everyone.

The thing is, I mainly did this for DeviceId to be consistent with ServerName, which is a wrapper around str and works the exact same way.

now we have plenty of places where we need to box the device id

This is purely from wanting to store it, not from moving it into a struct defined in Ruma, right?

Perhaps wrapping a Cow might work

Wrapping a Cow would work I guess, although it would add lots of pointless (though hidden) branching.

or the unicase case (drum) mentioned over here sounds nice as well.

That's what we had on master previously (though never in a released version). You probably didn't notice because none of the types in the other library crates ever used the borrowed version. I undid that in #131 because it didn't turn out to be a very nice solution at all – it is not at all common to use ThingRef<'_> instead of &Thing in other libs and the latter also can't deref into the former like &String derefs into &str.

What makes you dislike Box<DeviceId>?

[poljar]

This is purely from wanting to store it, not from moving it into a struct defined in Ruma, right?

Depends what you mean by that. Some Ruma structs now take a Box<DeviceId> as well, for example DeviceIdOrAllDevices for the to-device requests. Though we're mostly going to use an already boxed DeviceId as the source to fill out this struct.

What makes you dislike Box<DeviceId>?

Just that it requires more transformations on the client side than what we had before, e.g. if we want to store a device id we need to be aware of the fact that we need to box it. Seeing Box<DeviceId> all over the place is a downgrade from my point of view as well.

I understand that there isn't much difference between the need to box stuff now and between the need to clone a string like we had before, and if I want to hide the fact that it's a Box<T> I can just make another type alias.

It isn't a huge friction point feel free to close this if you don't think that this is worthwhile.

[jplatte]

Depends what you mean by that. Some Ruma structs now take a Box<DeviceId> as well

Yeah, the plan was to reintroduce Incoming / Outgoing so that you don't need owned identifiers to serialize requests in a client / responses in a server. Not sure whether that will make it into ruma 0.1 though.

Just that it requires more transformations on the client side than what we had before, e.g. if we want to store a device id we need to be aware of the fact that we need to box it.

I've tried making .into() "just work", and I think I commented on the ruma update PR in matrix-rust-sdk that you probably want to use that over .to_owned().into_boxed_str() or .to_string().into_boxed_str() most of the time. Does that help?

Seeing Box<DeviceId> all over the place is a downgrade from my point of view as well.

Unfortunately I don't see a good way of allowing DeviceId to be owned and &DeviceId to be constructible from &str at the same time, and not supporting the latter seems wrong to me when we can. (lets us support this)

I understand that there isn't much difference between the need to box stuff now and between the need to clone a string like we had before, and if I want to hide the fact that it's a Box<T> I can just make another type alias.

If you can come up with a good name I'm totally up to including such an alias in ruma-identifiers.

feel free to close this if you don't think that this is worthwhile.

I always appreciate feedback like this, so I won't just close it :)

[poljar]

I've tried making .into() "just work", and I think I commented on the ruma update PR in matrix-rust-sdk that you probably want to use that over .to_owned().into_boxed_str() or .to_string().into_boxed_str() most of the time. Does that help?

Yes you did, @DevinR528 adopted the PR to use it, so as I said it isn't that much different to the clone() calls we had.

If you can come up with a good name I'm totally up to including such an alias in ruma-identifiers.

It seems that I can't, DevId vs DeviceId came to me but it's not very nice, Haskells DeviceId' might be nice if we could have it, but might also confuse people. 😅

[jplatte]

It seems that I can't, DevId vs DeviceId came to me but it's not very nice

Same for DeviceId vs DeviceIdent or DeviceIdentifier, which I came with earlier but forgot to mention here. Both your and my option would kind of mirror str vs String, but I still think they'd be confusing :/

Haskells DeviceId' might be nice if we could have it

Yeah if we could have it and it was a convention (or we'd try to make it one) that would be kind of nice. I doubt Rust will ever allow this, though. Non-ascii idents also seem better in any way (in addition to still not being stable).

CC @iinuwa @florianjacob @sparky8251 @aledomu

[iinuwa]

Could we use a lower case identifier for the str newtype and Pascal case for the Box one? Would look kind of weird, but it would make it look like the str primitive/String type pairing.

[jplatte]

You mean snake_case, as in id: &device_id? Or really just lowercasing the type as in id: &deviceid?

[iinuwa]

I don't know, neither looks particularly good to me.

[jplatte]

@poljar What's your current opinion on this after having Box<DeviceId> in matrix-rust-sdk for a bit?

[poljar]

An alias would be nice but I think it's fine as is as well.

[jplatte]

Okay, what about DeviceIdBox (and ServerNameBox), like the FnBox Rust had for some time?

[poljar]

Futures went the other way with BoxFuture. I think I'm fine BoxDeviceId as well as DeviceIdBox.

[jplatte]

Added the type aliases in a842c5c.