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

[naskya]

• Closes feature request: add ability to use proxy #56

[andyblarblar]

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]

Okay! I applied the fixes suggested by cargo clippy.

Clippy also warned me that this response is ()

rust-web-push/examples/simple_send.rs

Lines 85 to 86 in 010cac3

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

because send returns Result<(), WebPushError>

rust-web-push/src/clients/hyper_client.rs

Line 40 in 010cac3

async fn send(&self, message: WebPushMessage) -> Result<(), WebPushError> {

which comes from request_builder::parse_response.

rust-web-push/src/clients/hyper_client.rs

Lines 79 to 87 in 010cac3

	let response = request_builder::parse_response(response_status, body.to_vec());
	debug!("Response: {:?}", response);
	if let Err(WebPushError::ServerError(None)) = response {
	Err(WebPushError::ServerError(retry_after))
	} else {
	Ok(response?)
	}

rust-web-push/src/clients/request_builder.rs

Lines 73 to 74 in 010cac3

	/// Parses the response from the push service, and will return `Err` if the request was bad.
	pub fn parse_response(response_status: StatusCode, body: Vec<u8>) -> Result<(), WebPushError> {

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.

rust-web-push/src/clients/request_builder.rs

Lines 165 to 168 in 010cac3

	#[test]
	fn parses_a_successful_response_correctly() {
	assert_eq!(Ok(()), parse_response(StatusCode::OK, vec![]))
	}

[naskya]

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]

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!