Skip to content

Fix Clippy warning in Routable macro#1975

Merged
ealmloff merged 2 commits intoDioxusLabs:mainfrom
tigerros:fix-routable-clippy-warning
Feb 24, 2024
Merged

Fix Clippy warning in Routable macro#1975
ealmloff merged 2 commits intoDioxusLabs:mainfrom
tigerros:fix-routable-clippy-warning

Conversation

@tigerros
Copy link

Routable creates types which derive PartialEq, but can also derive Eq. Clippy complains about this. This PR makes them derive Eq as well. If deriving only PartialEq is intentional let me know and I'll add an #[allow] instead.

Copy link
Member

@ealmloff ealmloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the error types that the routable macro generates comes from the error type in different router traits including FromRouterSegments. Deriving Eq on the error enum requires each of those error types to implement Eq which causes this code to stop compiling on this branch:

#[rustfmt::skip]
#[derive(Routable, Clone, PartialEq)]
enum Route {
    #[route("/:id")]
    Blog { id: MyCustomType },
}

#[derive(Clone, Debug, PartialEq)]
struct MyCustomType;

impl std::fmt::Display for MyCustomType {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "My Custom Type")
    }
}

impl dioxus_router::routable::FromRouteSegment for MyCustomType {
    // Any error type that implements PartialEq but not Eq currently compiles, but will fail to compile with this change
    type Err = f32;

    fn from_route_segment(route: &str) -> Result<Self, Self::Err> {
        if route == "my-custom-type" {
            Ok(Self)
        } else {
            Err(0.0)
        }
    }
}

#[component]
fn Blog(id: MyCustomType) -> Element {
    rsx! {
        h1 { "How to make: " }
        p { "{id}" }
    }
}

@tigerros
Copy link
Author

Ok, I've just added an #[allow(clippy::derive_partial_eq_without_eq)] then.

Copy link
Member

@ealmloff ealmloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@ealmloff ealmloff merged commit 451a8f2 into DioxusLabs:main Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants