pimeys / rust-web-push

A Web Push library for Rust
Apache License 2.0
114 stars 21 forks source link

Documentation/Example improvements #59

Closed naskya closed 5 months ago

naskya commented 5 months ago

I think there is room for improvement in the documentation and the examples. If the maintainer(s) agree with my suggestion, I’m willing to open a new pull request to fix it.

1. println! usage in the example code

The use of the println! macro is a bit weird here, since this response is (). We should just remove the print statement or use .map_err() / if let Err(err) = response { ... } / etc. to print only the errors (in case it fails). https://github.com/pimeys/rust-web-push/blob/010cac32a618379ec11c5d18912f8c70567e8484/examples/simple_send.rs#L85-L86 ref: https://github.com/pimeys/rust-web-push/pull/57#issuecomment-2087176615

2. The example in README does not compile

The example in README does not compile because web_push::clients is a private module. Since IsahcWebPushClient is re-exported, we can simply remove the second use to make it compilable. https://github.com/pimeys/rust-web-push/blob/40febe4085e3cef9cdfd539c315e3e945aba0656/README.md?plain=1#L26-L27 https://github.com/pimeys/rust-web-push/blob/40febe4085e3cef9cdfd539c315e3e945aba0656/src/lib.rs#L55-L56 https://github.com/pimeys/rust-web-push/blob/40febe4085e3cef9cdfd539c315e3e945aba0656/src/lib.rs#L67

3. There is an alternative solution to the OpenSSL requirement

README says

https://github.com/pimeys/rust-web-push/blob/40febe4085e3cef9cdfd539c315e3e945aba0656/README.md?plain=1#L13-L15

but system OpenSSL is not a strict requirement.

We can use vendored feature of openssl crate to make it work without installing OpenSSL on host (although vendored feature requires other dependencies, but most of them are covered by build-essential on Debian/Ubuntu, base-devel on Arch Linux, groupinstall "Development Tools" on Fedora/Red Hat distros, etc). I can provide a minimal working example of this (in a sample GitHub workflow run or something like that) if needed, but I already confirmed this by actually using web-push and openssl crates in our project.

Thanks a lot for maintaining this project!

andyblarblar commented 5 months ago

These look good to me. I didn't realize the readme wasn't updated when we made breaking changes, so that's a good catch. The vendored openssl is also useful, although if I remember correctly it wasn't working on windows a few years ago when I wrote this guide. It's probably been fixed since then so I'm not concerned.