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

Exposes surf's with_http_client #94

Merged
merged 3 commits into from
Oct 13, 2021
Merged

Conversation

ctrlaltf24
Copy link
Contributor

@ctrlaltf24 ctrlaltf24 commented Aug 3, 2021

Description

Exposed Surf's with_http_client to allow users to more granularity in their HTTP clients. One use case is creating a isahc HttpClient with HTTP basic auth

let http_client = isahc::HttpClientBuilder::new().authentication(Authentication::basic()).build()?;
let http_client = IsahcClient::from_client(http_client);
let client = Client::new("username:password@domain.com", "database").with_auth("user", "pass").with_http_client(http_client);

Maintenance considerations

If the project moves away from surf this function will need to be under a surf feature. This function does make moving away from surf a semver major change, looking at #92 it appears a sermver breaking change when moving away from surf is planned.

Checklist

  • Formatted code using cargo fmt --all
  • Linted code using clippy cargo clippy --all-targets --all-features -- -D warnings
    • NOTE: There are a couple existing errors trait objects without an explicit dyn are deprecated.
  • Updated README.md using cargo readme -r influxdb -t ../README.tpl > README.md
  • Reviewed the diff. Did you leave any print statements or unnecessary comments?
  • Any unfinished work that warrants a separate issue captured in an issue with a TODO code comment

@ctrlaltf24 ctrlaltf24 marked this pull request as draft August 3, 2021 22:24
@ctrlaltf24 ctrlaltf24 marked this pull request as ready for review August 3, 2021 22:36
@ctrlaltf24
Copy link
Contributor Author

Style checks fail on existing code.
Compile check failed on socket2 (not even in the influxdb codebase). Perhaps the runner needs to update to latest rust (rust-lang/rust#49146 (comment)) ?

@ctrlaltf24
Copy link
Contributor Author

@Empty2k12 thoughts?

@msrd0
Copy link
Collaborator

msrd0 commented Oct 4, 2021

Thanks for your PR!

Please merge/rebase main. The compilation failure should go away. Also, #92 has been merged.

@ctrlaltf24
Copy link
Contributor Author

Exposed under the "surf" feature. Do you prefer a separate function with_http_client (as is), or a separate new new_with_http_client?

@msrd0
Copy link
Collaborator

msrd0 commented Oct 4, 2021

I think I prefer the former. Is there a reason why we don't make a client-agnostic Client::with_http_client(c: HttpClient) that just replaces self.client directly?

@ctrlaltf24
Copy link
Contributor Author

ctrlaltf24 commented Oct 4, 2021

I think I prefer the former. Is there a reason why we don't make a client-agnostic Client::with_http_client(c: HttpClient) that just replaces self.client directly?

Fair point! Updated to match.

@Empty2k12
Copy link
Collaborator

@msrd0 can this be merged?

@msrd0
Copy link
Collaborator

msrd0 commented Oct 7, 2021

@Empty2k12 I was hoping for the CI to run first. No idea why it doesnt, I contacted GitHub Support on Monday asking why they've disabled actions on this repo.

@msrd0 msrd0 merged commit b38a5e6 into influxdb-rs:main Oct 13, 2021
@ctrlaltf24 ctrlaltf24 deleted the with_http_client branch October 13, 2021 16:19
@msrd0 msrd0 added this to the 0.5.0 milestone Oct 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants