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

Documentation/Example improvements #59

Closed
naskya opened this issue Jun 1, 2024 · 1 comment · Fixed by #60
Closed

Documentation/Example improvements #59

naskya opened this issue Jun 1, 2024 · 1 comment · Fixed by #60

Comments

@naskya
Copy link
Contributor

naskya commented Jun 1, 2024

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).

let response = client.send(builder.build()?).await?;
println!("Sent: {:?}", response);

ref: #57 (comment)

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.

rust-web-push/README.md

Lines 26 to 27 in 40febe4

use web_push::*;
use web_push::clients::isahc_client::IsahcWebPushClient;

rust-web-push/src/lib.rs

Lines 55 to 56 in 40febe4

#[cfg(feature = "isahc-client")]
pub use crate::clients::isahc_client::IsahcWebPushClient;

mod clients;

3. There is an alternative solution to the OpenSSL requirement

README says

rust-web-push/README.md

Lines 13 to 15 in 40febe4

## Requirements
Clients require an async executor. System Openssl is needed for compilation.

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
Copy link
Collaborator

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.

naskya added a commit to naskya/rust-web-push that referenced this issue Jun 1, 2024
naskya added a commit to naskya/rust-web-push that referenced this issue Jun 1, 2024
naskya added a commit to naskya/rust-web-push that referenced this issue Jun 1, 2024
naskya added a commit to naskya/rust-web-push that referenced this issue Jun 1, 2024
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 a pull request may close this issue.

2 participants