Skip to content
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.

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

Derive Debug for all structs #17

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

h7x4
Copy link
Collaborator

@h7x4 h7x4 commented Jun 19, 2023

Let all structs derive debug for ease of debugging.

The only thing that I feel is a little questionable about this is making UsbInterfaceHandler and UsbDeviceHandler into a supertraits of Debug. Although not semantically correct, it is meant to force UsbDevice.device_handler: Option<Arc<Mutex<Box<dyn UsbDeviceHandler + Send>>>> to be debuggable, so UsbDevice can be so as well. Would it be better to maintain a manual implementation here?

Let all structs derive debug for ease of debugging.
@jiegec
Copy link
Owner

jiegec commented Jun 19, 2023

I don't know what's the best practice, but it might break existing code not impl-ing Debug?

@h7x4
Copy link
Collaborator Author

h7x4 commented Jun 19, 2023

... it might break existing code not impl-ing Debug?

That's a good call. Do you think it would be better to create a manual implementation? I guess something like derivative also is an option, at the cost of introducing a new dependency.

@jamesadevine
Copy link
Collaborator

jamesadevine commented Jun 20, 2023

Could this be a package configuration option instead?

@h7x4
Copy link
Collaborator Author

h7x4 commented Jun 20, 2023

That works for me. I've never heard of debug being a used as a configuration option though. Is the intention to include the extra dependency only for those that need it?

@jamesadevine
Copy link
Collaborator

I don't think it is a big deal to derive the debug struct personally.

@h7x4 h7x4 marked this pull request as draft July 17, 2023 12:02
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.

None yet

4 participants