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

Various changes #5

Merged
merged 4 commits into from
Jun 26, 2022
Merged

Various changes #5

merged 4 commits into from
Jun 26, 2022

Conversation

wezm
Copy link
Contributor

@wezm wezm commented Jun 18, 2022

I made some fairly bold changes to the code. I will understand if you don't want to merge them but figured I'd raise the PR for you to consider. The commits are pretty self-contained and do the following:

  • Drop the thiserror dependency in favour of manually implementing the std::error::Error impl.
  • Drop the byteorder dependency in favour of the functions in std.
  • Fix a warning in some example code.
  • Avoid unwrapping in the send macro:
    • This seemed like a bit of a footgun since the unwrap was hidden.
    • I used this change to potentially avoid re-panicking inside the panic handler.
  • Drop the write_output method since send_message was identical.

I'd also like to run rustfmt on the code, if you're ok with that I'll do it in a separate PR.

wezm added 4 commits June 18, 2022 14:44
The example was binding the value from the event loop to a variable
named Null. The subsequent catch-all (_) could never be reached.
@neon64
Copy link
Owner

neon64 commented Jun 18, 2022

Hey! As you can probably tell I haven't been super-actively maintaining this library, but I'm very happy to accept your contributions. These changes mostly sound reasonable! The redundant byteorder dep is interesting - I think I must have originally written this before the equivalent stdlib functions were added.

The one change I'm not immediately sold on is dropping the thiserror dependency. My understanding is that thiserror reduces some of the boilerplate of writing the std::error::Error and Display impls, but isn't visible at all in the public API. Admittedly, this library is so small we're only saving ~20 lines of code. Is there a particular advantage you see in avoiding the thiserror dependency?

@wezm
Copy link
Contributor Author

wezm commented Jun 19, 2022

Admittedly, this library is so small we're only saving ~20 lines of code. Is there a particular advantage you see in avoiding the thiserror dependency?

I think fewer dependencies is generally better (when it makes sense) as there's fewer things outside of your control. Nothing against thiserror, just didn't seem worth pulling in an extra dependency (and its dependencies) to save 20 lines as you say.

thiserror v1.0.31
└── thiserror-impl v1.0.31 (proc-macro)
    ├── proc-macro2 v1.0.39
    │   └── unicode-ident v1.0.1
    ├── quote v1.0.18
    │   └── proc-macro2 v1.0.39 (*)
    └── syn v1.0.96
        ├── proc-macro2 v1.0.39 (*)
        ├── quote v1.0.18 (*)
        └── unicode-ident v1.0.1

It might also help this crate compiler faster as there's no proc macros involved now—and everyone loves faster Rust compile times :)

@neon64
Copy link
Owner

neon64 commented Jun 26, 2022

Thanks. Indeed that does look quite heavy with all the recursive deps. I'm curious how many of those would already be included your average Rust project (e.g.: other crates like serde also pull in syn, quote, etc...). I suspect different versions could pose a problem (i.e.: multiple separate copies of syn because different projects request different versions).

I think with that in mind I'll just merge this PR.

@neon64 neon64 merged commit bce15d1 into neon64:master Jun 26, 2022
@wezm wezm deleted the drop-deps branch June 26, 2022 23:05
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