search

pimeys / rust-web-push

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

Add ability to create a new WebPushClient from a custom Isahc/hyper client (#56) #57

Closed naskya closed 4 months ago

naskya commented 4 months ago
andyblarblar commented 4 months ago

Thanks for the contribution! Would you mind removing the unused import causing the CI to fail? I don't think this is actually from your changes, but either way it would be nice to fix while we're here.

naskya commented 4 months ago

Okay! I applied the fixes suggested by cargo clippy.

Clippy also warned me that this response is ()

https://github.com/pimeys/rust-web-push/blob/010cac32a618379ec11c5d18912f8c70567e8484/examples/simple_send.rs#L85-L86

because send returns Result<(), WebPushError>

https://github.com/pimeys/rust-web-push/blob/010cac32a618379ec11c5d18912f8c70567e8484/src/clients/hyper_client.rs#L40

which comes from request_builder::parse_response.

https://github.com/pimeys/rust-web-push/blob/010cac32a618379ec11c5d18912f8c70567e8484/src/clients/hyper_client.rs#L79-L87

https://github.com/pimeys/rust-web-push/blob/010cac32a618379ec11c5d18912f8c70567e8484/src/clients/request_builder.rs#L73-L74

Part of me wonders if this is unintentional and you may actually want to return http::Response rather than (), but a function in the unit test implies this is intentional, so I’m not quite sure how I should fix this.

https://github.com/pimeys/rust-web-push/blob/010cac32a618379ec11c5d18912f8c70567e8484/src/clients/request_builder.rs#L165-L168

naskya commented 4 months ago

Having said that, changing the return value of a function would cause a breaking change, and it may be too much to address that warning in this single merge request, so I personally think we should ignore it for now.

andyblarblar commented 4 months ago

Yeah, I agree. I think it's a reasonable pattern to use to just use a Result with a Unit in the Ok when you only want to indicate an error, but the way the code is used here is odd. Given the functions docs and the unit test it does look like this is intential, and the function is just used to check for bad HTTP codes. Not ideal, but pretty niche tbh given how this library is normally used.

Now that all the CI passes, I'll merge this in. Thanks for the help again!