refactor: port discussions.py off requests onto PyGithub GraphQL by jmeridth · Pull Request #800 · github-community-projects/issue-metrics

jmeridth · 2026-07-28T04:58:35Z

Closes #790
Relates to #799

Proposed Changes

Port get_discussions() from a direct requests.post GraphQL call to PyGithub's github_connection.requester.graphql_query, so PyGithub owns all HTTP (auth, base URL, and GitHub Enterprise /api/graphql routing) and requests is no longer a direct dependency.

discussions.py: signature is now get_discussions(github_connection, search_query). The manual status-code and errors checks are gone because graphql_query raises GithubException on both HTTP and GraphQL errors, which now propagates.
issue_metrics.py: the single caller passes the existing github_connection.
test_discussions.py: mocks the PyGithub GraphQL call instead of requests.post, adds a pagination-cursor assertion, and replaces the two removed error-path tests with an exception-propagation test.
pyproject.toml / uv.lock: drop requests and types-requests as direct dependencies. requests remains transitively via PyGithub.

Proof it works: make test passes (206 tests, 100% coverage, discussions.py at 100%). A spike confirmed graphql_query returns (headers, {"data": {...}}) and derives the correct GHE endpoint from the connection base URL (https://ghe.example.com/api/graphql).

Risk + AI role: Low. Narrow surface with one internal caller. AI-assisted implementation, independently reviewed across four models.

Behavioral notes (pre-existing inconsistencies, tracked in #799):

Discussions now route via the connection base URL, consistent with every other call in the tool. GitHub App users on GHE must set GITHUB_APP_ENTERPRISE_ONLY=true for discussions to reach GHE (they already needed it for all other calls to work).
The request timeout is now PyGithub's default rather than the discussions-only 60s, matching the rest of the tool.

Review focus: The removal of the manual error checks (relying on GithubException to propagate), and the GHE routing note for GitHub App auth.

Readiness Checklist

Author/Contributor

If documentation is needed for this change, has that been included in this pull request
run make lint and fix any issues that you have introduced
run make test and ensure you have test coverage for the lines you are introducing

Closes #790 ## What/Why Move get_discussions() from a direct requests.post GraphQL call to PyGithub's github_connection.requester.graphql_query, so PyGithub owns all HTTP (auth, base URL, GitHub Enterprise /api/graphql routing) and requests is no longer a direct dependency. ## Proof it works uv run pytest -> 206 passed, 100% coverage (discussions.py 100%). make lint clean (flake8, isort, pylint 10.00/10, mypy, black). Spike verified graphql_query returns (headers, {"data": {...}}) and derives the GHE GraphQL endpoint from base_url (https://ghe.example.com/api/graphql). ## Risk + AI role Low. Signature changes to get_discussions(github_connection, search_query) with one internal caller updated. Manual status/errors ValueError checks are removed since graphql_query raises GithubException itself. AI-authored (Claude Opus 4.8) and reviewed by four models (Opus, Sonnet, Fable, Haiku). Behavioral notes (pre-existing inconsistencies, tracked in #799): App-auth GHE users must set GITHUB_APP_ENTERPRISE_ONLY=true for discussions to reach GHE (now consistent with every other call); the request timeout is now PyGithub's default rather than the old discussions-only 60s. ## Review focus The removal of the manual error checks (relying on GithubException to propagate) and the GHE routing note for GitHub App auth. Signed-off-by: jmeridth <jmeridth@gmail.com>

Copilot

Pull request overview

This PR refactors the Discussions fetching path to use PyGithub’s requester.graphql_query instead of a direct requests.post, so HTTP concerns (auth, base URL, GitHub Enterprise routing) are owned by the existing Github connection rather than a separate client.

Changes:

Refactor get_discussions() to accept a Github connection and call github_connection.requester.graphql_query(...).
Update the single caller in issue_metrics.py and adjust unit tests to mock PyGithub’s GraphQL method (including a pagination cursor assertion).
Remove requests / types-requests as direct dependencies from pyproject.toml and uv.lock.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`discussions.py`	Switches Discussions GraphQL calls from `requests.post` to PyGithub’s `requester.graphql_query` and updates the function signature accordingly.
`issue_metrics.py`	Updates the discussions path to pass the existing `github_connection` into `get_discussions()`.
`test_discussions.py`	Mocks `requester.graphql_query`, adds a pagination-cursor assertion, and tests exception propagation.
`pyproject.toml`	Drops `requests` and `types-requests` as direct dependencies.
`uv.lock`	Updates the lock file to reflect removal of the direct `requests` / `types-requests` entries.

Comments suppressed due to low confidence (1)

discussions.py:18

The parameter type in the docstring still refers to github.Github, but the actual annotation/import uses Github from github. Updating the docstring keeps documentation consistent and avoids referencing a non-imported github module name.

    Args:
        github_connection (github.Github): An authenticated PyGithub connection.
            GitHub Enterprise routing is handled by the connection's base URL.

Closes #790 ## What/Why Address a PR review note: the docstring referenced `github.Github` and `List[Dict]` while the module imports `Github` directly and returns a plain list. Use `Github` and `list` to match the imported names and the convention in search.py. ## Proof it works Docstring-only change. pytest test_discussions.py -> 3 passed. pylint 10.00/10, mypy and black clean. ## Risk + AI role Low. Documentation only, no runtime change. AI-assisted. ## Review focus None needed beyond the wording. Signed-off-by: jmeridth <jmeridth@gmail.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Closes #790 ## What/Why Address a PR review note: construct the test GithubException with a dict `data` payload ({"message": "server error"}) to match the convention used across test_search.py, rather than a bare string. ## Proof it works pytest test_discussions.py -> 3 passed. black clean. ## Risk + AI role Low. Test-only change, no runtime impact. AI-assisted. ## Review focus None needed. Signed-off-by: jmeridth <jmeridth@gmail.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.

zkoppert

Looks great!

Copilot AI review requested due to automatic review settings July 28, 2026 04:58

jmeridth self-assigned this Jul 28, 2026

jmeridth added the Mark Ready When Ready Automatically mark draft PR ready when checks pass label Jul 28, 2026

github-actions Bot added the automation label Jul 28, 2026

Copilot started reviewing on behalf of jmeridth July 28, 2026 04:59 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Comment thread discussions.py

github-actions Bot marked this pull request as ready for review July 28, 2026 05:02

github-actions Bot requested a review from zkoppert as a code owner July 28, 2026 05:02

github-actions Bot removed the Mark Ready When Ready Automatically mark draft PR ready when checks pass label Jul 28, 2026

Copilot AI review requested due to automatic review settings July 28, 2026 05:04

Copilot started reviewing on behalf of jmeridth July 28, 2026 05:05 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Comment thread test_discussions.py

Copilot started work on behalf of jmeridth July 28, 2026 05:07 View session

Copilot AI review requested due to automatic review settings July 28, 2026 05:09

Copilot finished work on behalf of jmeridth July 28, 2026 05:09

Copilot started reviewing on behalf of jmeridth July 28, 2026 05:10 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Copilot started work on behalf of jmeridth July 28, 2026 05:13 View session

Copilot finished work on behalf of jmeridth July 28, 2026 05:14

zkoppert approved these changes Jul 29, 2026

View reviewed changes

jmeridth merged commit a98bcbf into main Jul 29, 2026
41 checks passed

jmeridth deleted the 790-port-discussions-to-pygithub-graphql branch July 29, 2026 18:01

Navigation Menu

Uh oh!

refactor: port discussions.py off requests onto PyGithub GraphQL - #800

refactor: port discussions.py off requests onto PyGithub GraphQL#800
jmeridth merged 3 commits into
mainfrom
790-port-discussions-to-pygithub-graphql

jmeridth commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

zkoppert left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jmeridth commented Jul 28, 2026

Proposed Changes

Readiness Checklist

Author/Contributor

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

zkoppert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants