Skip to content

Subscriber: Debug #45

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Closed
conradludgate opened this issue May 5, 2022 · 4 comments
Closed

Subscriber: Debug #45

conradludgate opened this issue May 5, 2022 · 4 comments

Comments

@conradludgate
Copy link
Contributor

Is there any reason why the HeirarchicalLayer needs the S: fmt::Debug bound? The code doesn't seem to use it anywhere
@davidbarsky
Copy link
Owner

davidbarsky commented May 5, 2022 •
edited
Loading

I don't exactly recall, but if I'm pretty sure that HierarchicalLayer only implements has an S: fmt::Debug bound when it itself is also Debug. This mimics how tracing_subscriber::fmt::Layer does things, albeit with a slightly different declaration.

Is this giving you any issues?

@conradludgate
Copy link
Contributor Author

Not directly, but it does make it harder to pair this layer with others that might not implement debug.

I think the debug requirement for Layer should be moved to the debug imply itself

@davidbarsky
Copy link
Owner

yeah, i'm not opposed to that change. i don't think that moving the debug implementation is a breaking change to a separate impl block, but i'll confirm.

@v3rmine v3rmine mentioned this issue May 8, 2022
Closed
@davidbarsky davidbarsky mentioned this issue May 26, 2022
Merged
davidbarsky added a commit that referenced this issue May 26, 2022
@davidbarsky
fix: remove unnecessary Debug bounds; fix panic (#46)
8f0e5ee 
Resolves tokio-rs/tracing#2112; #45.

This PR does two things:
- removes unnecessary `fmt::Debug` bounds on `HierarchicalLayer`, which makes it possible to compose `HierarchicalLayer` with other `HierarchicalLayer`s (#45).
- Checks whether another `HierarchicalLayer` has already placed `Data` in the extensions; skipping if it's already present. This prevents the panic reported in tokio-rs/tracing#2112.
@davidbarsky
Copy link
Owner

Resolved by #46.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@davidbarsky @conradludgate