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

Replace reqwest with http #99

Closed
XAMPPRocky opened this issue Jun 27, 2021 · 4 comments · Fixed by #297
Closed

Replace reqwest with http #99

XAMPPRocky opened this issue Jun 27, 2021 · 4 comments · Fixed by #297
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@XAMPPRocky
Copy link
Owner

XAMPPRocky commented Jun 27, 2021

In order to be more client agnostic, and allow using Octocrab in more places, I would like to remove reqwest and replace it entirely with http, tower, and hyper. Using the same approach as kube-rs.

If anyone is interested in taking this on, I would make a PR per endpoint or endpoint group, and not move all of them at once. Feel to ask on the issue or in the discussion board if you need help.

@jesseli2002
Copy link
Contributor

I'm learning Rust by building a CLI tool which interacts with Github, which is how I came across this crate and issue. (I'm also mainly working in embedded devices, not web stuff, so I'm doubly out of my element)

Forgive me if I'm misunderstanding/making some rookie mistake, but at least for the endpoints I was looking at, it looks like none of them directly depend on reqwest, instead with requests all eventually making their way to Octocrab::execute (which does take a reqwest::RequestBuilder). So to remove the dependency on reqwest, wouldn't this involve some small but breaking change to the Octocrab API, rather than a change to each endpoint individually? (i.e. I'm confused about the comment of making a PR per endpoint)

@IgnisDa
Copy link

IgnisDa commented Jan 13, 2023

@XAMPPRocky @jesseli2002 Any updates on this? Otherwise I would like to take a shot at this.

@XAMPPRocky
Copy link
Owner Author

@IgnisDa Thank you for your interest! (and sorry @jesseli2002 for the late reply) I'd say go for it, PRs are always welcome. What I said above is non longer applicable, as yes all the requests go through Octocrab::execute now, so it's an all or nothing kind of PR. I would just remove reqwest, see what breaks, and replace it with HTTP. If you run into any issues, feel free to post here and I'll try to keep a better eye on the issue to answer any questions you have.

@XAMPPRocky
Copy link
Owner Author

This change is now available in 0.20.0-alpha.0

@XAMPPRocky XAMPPRocky unpinned this issue Jun 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
3 participants