-
-
Notifications
You must be signed in to change notification settings - Fork 51
Description
Summary
Following the changes in 938f24c, to require an Issue to be opened prior to a PR I'm opening one.
I believe many of the calls that are made to the GitHub API can be internalized and simplified via the use of an existing library crate.
The octocrab crate is a modern, actively maintained Rust client for the GitHub API, built on top of reqwest but providing a high-level, idiomatic interface.
I strongly believe that API interactions that can be handled through a dedicated, battle-tested library should be, rather than relying on manual, low-level constructions.
I'm proposing to migrate the various manual reqwest-based calls to the GitHub API (e.g., fetching releases, commits) to use OctoCrab instead.
There are many benefits to this, among them:
- Improved Reliability: OctoCrab handles authentication, rate limiting(*), pagination, and error responses natively, reducing the risk of custom parsing errors (like the current messy deserialization that checks for "message" fields and specific strings for rate limits).
- Runtime Performance: Fewer manual string manipulations and header setups on every call; OctoCrab optimizes requests under the hood already.
- Maintainability Improvements: Abstracts away tedious boilerplate (e.g., no more custom
make_github_requestfunctions or ad-hoc URL formatting). This allows for trait-based interfaces if needed (much easier to test as well), promoting better Rust idioms over scattered procedural code. - Code Reduction: Eliminates redundant functions like
get_upstream_nightly,get_commits_for_nightly, anddeserialize_response, replacing them with concise OctoCrab methods
(e.g., OctoCrab's ReleasesHandler will be able to handle almost everything related to releases as a single structure, listing releases, creating paged-output (for display via list-remote), and pulling the different variations bob needs to support (git | head, nightly, stable, and older versions) and by either tag or commit - directly compatible with semver version(s)).
-
Better Error Handling: Built-in support for GitHub's API nuances, avoiding hacks like manual version string parsing or fragile JSON value inspections.
-
Future-Proofing: Easier to extend for new API features without reinventing the wheel, and it integrates well with async/await patterns already in use.
This is especially relevant given multiple users having requested we support older version of Debian/Ubuntu which use outdated
glibcversions and therefore fail to compile locally.
Using a GitHub specific client acts as a good stepping stone here - allowing users to choose which mirror they want to pull from in the meantime, and later allowing a smoother transition to using GitHub Releases via cross for cross-compilation or similar. -
rate-limiting* This is something I've personally encountered when running local tests or during development due to where and when we're actually checking for rate-lmits. Without the ability to pass in a GITHUB_TOKEN this becomes a major pain point for both developers and users alike, having a library that is able to correctly handle/parse a GITHUB_TOKEN and manage rate limits would be a significant improvement for both developers and for end-users.
Justification for Change
The current implementation relies on low-level reqwest calls wrapped in custom helpers, which leads to unnecessary complexity and fragility. For instance:
pub async fn make_github_request<T: AsRef<str> + reqwest::IntoUrl>(
client: &Client,
url: T,
) -> Result<String> {
let response = client
.get(url)
.header("user-agent", "bob")
.header("Accept", "application/vnd.github.v3+json")
.send()
.await?
.text()
.await?;
Ok(response)
}
pub async fn get_upstream_nightly(client: &Client) -> Result<UpstreamVersion> {
let response = make_github_request(
client,
"https://api.github.com/repos/neovim/neovim/releases/tags/nightly",
)
.await?;
deserialize_response(response)
}
pub async fn get_commits_for_nightly(
client: &Client,
since: &DateTime<Utc>,
until: &DateTime<Utc>,
) -> Result<Vec<RepoCommit>> {
let response = make_github_request(client, format!(
"https://api.github.com/repos/neovim/neovim/commits?since={since}&until={until}&per_page=100")).await?;
deserialize_response(response)
}
pub fn deserialize_response<T: DeserializeOwned>(response: String) -> Result<T> {
let value: serde_json::Value = serde_json::from_str(&response)?;
if value.get("message").is_some() {
let result: ErrorResponse = serde_json::from_value(value)?;
if result.documentation_url.contains("rate-limiting") {
return Err(anyhow!(
"Github API rate limit has been reach, either wait an hour or checkout https://github.com/MordechaiHadad/bob#increasing-github-rate-limit"
));
}
return Err(anyhow!(result.message));
}
Ok(serde_json::from_value(value)?)
}This approach duplicates effort (e.g., repeated header setups, manual URL construction with string formatting), lacks abstraction (no traits despite similar behaviors), and introduces error-prone parsing (e.g., ad-hoc checks for rate limits or version strings). It's a reinvention of what OctoCrab already provides, and does so with a clean, type-safe API. Migrating would streamline these into a few lines per call, making the code more readable and less prone to bugs.
Open to discussing implementation details on various aspects of course, such as handling authentication (OctoCrab supports tokens easily) or any specific API endpoints that might need custom tweaks.