Discussion: Should ID support From<Uuid>? about async-graphql

[nicolaiunrein]

In fact this concerns all ID conversions. If that is something desirable I will give it a shot. Should not be breaking existing implementations because all types implement From and Into themselves.

[nicolaiunrein]

Brilliant! @IcanDivideBy0 I much prefer your solution! Didn't consider removing Display for those types an option.

[sunli829]

Since you have this requirement, I think this feature makes sense. I'll add it now.😁

[nicolaiunrein]

Cool. I will see what I can with error_extensions.md on monday.

[IcanDivideBy0]

Correct me if I'm wrong but this feels unnecessary complicated.
By removing the impl std::fmt::Display for Cursor, we can then add

impl<T> From<T> for Cursor
where
    T: std::fmt::Display
{
    fn from(value: T) -> Self {
        Cursor(value.to_string())
    }
}

which removes the need for the ToGraphQLCursor trait ?

Same goes for ID, I don't think those types needs to implement display, Debug should be enough for general purpose

[IcanDivideBy0]

@sunli829 you probably forgot to remove the impl fmt::Display for ID ? or any other From where the type already impl fmt::Display ?

[nicolaiunrein]

I use Uuids as well and I think it is quite common. Maybe implement it for T: Into<Uuid> so that it works well with the new type pattern?

[phated]

@nicolaiunrein I think that would be a good idea. When converting between ID and Cursor, I need to do id.to_string().into() which isn't ideal.

[sunli829]

@nicolaiunrein How is this feature going to be implemented?😁

[nicolaiunrein]

Sorry, I am pretty busy at the moment with other projects. I will look at it tomorrow or on monday.

[nicolaiunrein]

Well, we can't implement generic From implementation due to conflicting implementations but I would suggest to add two traits:

pub trait ToGraphqlCursor {
    fn to_graphql_cursor(&self) -> Cursor;
}

impl<T> ToGraphqlCursor for T
where
    T: std::fmt::Display,
{
    fn to_graphql_cursor(&self) -> Cursor {
        Cursor(self.to_string())
    }
}

and

pub trait ToGraphqlID {
    fn to_graphql_id(self) -> ID;
}

impl<T> ToGraphqlID for T
where
    T: std::fmt::Display,
{
    fn to_graphql_id(self) -> ID {
        ID(self.to_string())
    }
}

which when imported give you two methods to_graphql_cursor and to_graphql_id on anything that implements std::fmt::Display and allows you to instead of calling id.to_string().into() just id.to_graphql_cursor. Now this can be quite handy when you implement your own ID type as well:

pub struct MyID(Uuid);
impl std::fmt::Display for MyID { ... }

// somewhere else
let id = my_id.to_graphql_id();

What do you think?

[sunli829]

I think it's good, it's really easier to use. 😁

In addition, can you add this error_extensions.md when you are free? I think you know more about this than me.

I have been working on a new parser for the past few days and it is now complete. You can upgrade to 1.11.0 to see if there is a problem with your current project.

Thanks again, ToGraphqlID and ToGraphqlCursor I will add later. 😁

[sunli829]

Thank you. 😁

[sunli829]

@nicolaiunrein It has been implemented in version 1.11.1.

[IcanDivideBy0]

IcanDivideBy0@690f2b4

[IcanDivideBy0]

In the end it's even simpler

    let cursor: Cursor = "foo".into();

    let my_uuid = uuid::Uuid::parse_str("936DA01F9ABD4d9d80C702AF85C822A8").unwrap();
    let cursor: Cursor = my_uuid.into();

[sunli829]

@IcanDivideBy0 Seems to require generic specialization?

36 | impl<T: Display> From for ID {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: conflicting implementation in crate core:
- impl std::convert::From for T;

[sunli829]

@nicolaiunrein Is this the reason?

[sunli829]

Sorry, you actually answered the question. 😊@nicolaiunrein

[IcanDivideBy0]

@sunli829 let me send the PR

[sunli829]

Weird things, you can compile and pass. I want to see how your modification differs from mine.😳

[sunli829]

Thanks, I understand, because ID also implements Display, so it will conflict. I think the implementation of Display can indeed be removed, which is even simpler. Thank you for your PR. 😁

[IcanDivideBy0]

@nicolaiunrein ID and Cursor now properly implements From<fmt::Display> on master.
@sunli829 I let you decide if this worth a release :)

[sunli829]

Let me do it, just change the version number. 😂

[nicolaiunrein]

In addition, can you add this error_extensions.md when you are free? I think you know more about this than me.

@sunli829 I wrote something but it got quite lengthy. Please review.

[sunli829]

Thank you so much. I translated the Chinese version later. 🍺

[sunli829]

@nicolaiunrein The documentation is too good, much better than mine. 😂