Skip to content

fix: missing_debug_implementations #41

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JonathanWoollett-Light
Copy link
Contributor

Reason for This PR

It is good for public types to have std::fmt::Debug implementations.

Description of Changes

  1. Warns missing_debug_implementations lint.
  2. Adds Debug implementations.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any newly added unsafe code is properly documented.
  • Any user-facing changes are mentioned in CHANGELOG.md.

@JonathanWoollett-Light JonathanWoollett-Light self-assigned this Jul 6, 2023
@JonathanWoollett-Light JonathanWoollett-Light force-pushed the missing_debug_implementations branch 2 times, most recently from b8f3872 to 22eca8e Compare September 20, 2023 13:32
Public types should have `std::fmt::Debug` implementations.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
@JonathanWoollett-Light JonathanWoollett-Light force-pushed the missing_debug_implementations branch from 22eca8e to 2003663 Compare September 21, 2023 08:40
.field("server_id", &self.server_id)
.field("prefix", &self.prefix)
.field("media_type", &self.media_type)
.field("routes", &"{ .. }")

Choose a reason for hiding this comment

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

changing this to : .field("routes", &self.routes)

and implementing Debug for EndpointHandler which just has :

impl std::fmt::Debug for dyn EndpointHandler{
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.debug_struct("EndpointHandler")
        .field("handle_request",&"{..}}")
        .finish()
    }
}

can improve the debug prints to give us the routes strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you provide the full code for this change, I am not convinced this is a good solution.

Choose a reason for hiding this comment

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

This is what I tried:

diff --git a/src/router.rs b/src/router.rs
index 4676459..cc80bf9 100644
--- a/src/router.rs
+++ b/src/router.rs
@@ -16,6 +16,14 @@ pub trait EndpointHandler<T>: Sync + Send {
     fn handle_request(&self, req: &Request, arg: &T) -> Response;
 }
 
+impl<T> fmt::Debug for dyn EndpointHandler<T> + Send + Sync + 'static{
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("EndpointHandler")
+            .field("handle_request", &"{ .. }")
+            .finish()
+    }
+}
+
 /// An HTTP routes structure.
 pub struct HttpRoutes<T> {
     server_id: String,
@@ -31,7 +39,7 @@ impl<T> fmt::Debug for HttpRoutes<T> {
             .field("server_id", &self.server_id)
             .field("prefix", &self.prefix)
             .field("media_type", &self.media_type)
-            .field("routes", &"{ .. }")
+            .field("routes", &self.routes)
             .finish()
     }
 }

and the output for this shows:

HttpRoutes {
    server_id: "Mock_Server",
    prefix: "/api/v1",
    media_type: ApplicationJson,
    routes: {
        "GET:/api/v1/func1": EndpointHandler {
            handle_request: "{ .. }",
        },
    },
}

vs below with existing code in PR

HttpRoutes {
    server_id: "Mock_Server",
    prefix: "/api/v1",
    media_type: ApplicationJson,
    routes: "{ .. }",
}

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants