feat: add GitHub API rate limit awareness#54
Conversation
|
@Jiya3177 Thanks for you contribution , I'm on it will review it shortly |
Hell1213
left a comment
There was a problem hiding this comment.
Review: GitHub API rate limit awareness
Hi @Jiya3177, thanks for this PR! The rate limit awareness is well-implemented.
Verified: Tests pass (77/77), ruff checks clean.
Suggestion: _get_rate_limit() calls subprocess.run directly without checking if gh is available. If gh is not installed, the error message from subprocess wont be user-friendly. Consider adding self._require_gh() or self._check_gh() at the start of _get_rate_limit() to give a clear error message.
Important: PR #50 by @Achiever199 also modifies src/oss_dev/providers/github/client.py (adds caching layer). Both PRs touch _run_gh and the class methods. There will be merge conflicts – whoever merges second will need to rebase and reconcile changes to _run_gh, fetch_issue, list_issues, create_pr, etc.
Hell1213
left a comment
There was a problem hiding this comment.
Rate limit logic looks solid. The tests cover the key cases (zero quota, low quota, malformed response, no recursion on rate_limit endpoint). One small thing - _get_rate_limit() uses subprocess.run directly instead of going through _run_gh, so if gh is not installed the error wont be as clear. Not a blocker though.
Summary
Add rate-limit awareness to
GitHubCLIProviderbefore GitHub API calls.Related Issue
Fixes #37
Type of Change
Testing
uv run ruff checkpassesuv run mypypassesuv run pytestpassesManual testing:
PYTHONPATH=src python3 -m pytest tests/oss/test_github_cli_provider.pypython3 -m ruff check src/oss_dev/providers/github/client.py tests/oss/test_github_cli_provider.pyDescription
Added rate-limit checks before
gh apicalls inGitHubCLIProvider.This change:
gh api rate_limitoutput.ProviderErrormessages.gh api rate_limit.Added focused tests with mocked
subprocess.runso no real GitHub API calls are made.