reown-com / a2

An Asynchronous Apple Push Notification (apns2) Client for Rust
MIT License
151 stars 53 forks source link

Client: Generic type arguments #4

Closed dbrgn closed 6 years ago

dbrgn commented 6 years ago

Edit: Sorry, submitted too early due to a new keyboard :)

The Client::token signature currently looks like this:

    pub fn token<S, R>(
        pkcs8_pem: R,
        key_id: S,
        team_id: S,
        handle: &Handle,
        endpoint: Endpoint,
    ) -> Result<Client, Error>
    where
        S: Into<String>,
        R: Read,
{

This requires that key_id and team_id have the same type that implements Into.

What you want is probably this:

    pub fn token<S, R>(
        pkcs8_pem: R,
        key_id: S,
        team_id: T,
        handle: &Handle,
        endpoint: Endpoint,
    ) -> Result<Client, Error>
    where
        S: Into<String>,
        T: Into<String>,
        R: Read,
{

This way, the two types can differ, as long as they both impl Into.

dbrgn commented 6 years ago

Also, ToString (https://doc.rust-lang.org/std/string/trait.ToString.html) might be more appropriate than Into<String>, but I'm not 100% sure about that.

pimeys commented 6 years ago

A good point. I'll make the changes tomorrow when I have some time.

pimeys commented 6 years ago

Changed. I'll stick with the Into because I don't see much reason for ToString here. Yeah, of course the main use case here is to allow using &str or String, not that much anything else. Maybe even Cow would suffice.

dbrgn commented 6 years ago

You do need an owned/allocated version of the string though, right?

I think Into is fine.

pimeys commented 6 years ago

Yeah, you keep the client alive a long time so forcing it into a lifetime doesn't make sense. Also you hopefully create connections rarely, so allocating from the heap doesn't hurt.