Skip to content

access GraphQL urls from the designated graphql helper class#3686

Merged
MoralCode merged 1 commit intomainfrom
ntdn/consolodate_github_graphql_urls1
Feb 23, 2026
Merged

access GraphQL urls from the designated graphql helper class#3686
MoralCode merged 1 commit intomainfrom
ntdn/consolodate_github_graphql_urls1

Conversation

@MoralCode
Copy link
Copy Markdown
Collaborator

Description
This is essentially the same as #3685, but for the github graphQL API (another small step towards resolving #3277)

Notes for Reviewers
No testing done yet, pretty trivial change to set the pattern for how i want things to work with building URLs in the future

Signed commits

  • Yes, I signed my commits.

Copy link
Copy Markdown
Collaborator

@shlokgilda shlokgilda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the @staticmethod approach. you don't need an instance just to get the URL.
It's actually better than what #3685 does on the REST side (where _base_url() is an instance method that's never used). Might be worth aligning the REST PR to follow this same pattern? Thoughts?

@MoralCode
Copy link
Copy Markdown
Collaborator Author

So i did graphQL in a different way because graphQL queries dont really rely on url paths or parameters, they send the information for the request in the body.

I worry that by exposing the baseURL for REST in a similar way, consumers of this class may be encouraged to build their own queries (although i guess if theyre using the correct baseurl its probably fine????

@shlokgilda
Copy link
Copy Markdown
Collaborator

Yeah makes sense, keeping them different is the right call. LGTM.

@sgoggins
Copy link
Copy Markdown
Collaborator

So i did graphQL in a different way because graphQL queries dont really rely on url paths or parameters, they send the information for the request in the body.

I worry that by exposing the baseURL for REST in a similar way, consumers of this class may be encouraged to build their own queries (although i guess if theyre using the correct baseurl its probably fine????

This is not a comment with any valence. The REST API's rate limits are published and clear. The GraphQL API limits are generally greater, however we have observed they are also severely throttled from time to time. So, this is a "the more you know" sort of comment. This is the kind of thing we learn when collecting mass amounts of data for a while.

@MoralCode MoralCode requested a review from sgoggins February 19, 2026 22:56
Copy link
Copy Markdown
Collaborator

@sgoggins sgoggins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much more robust approach.

@sgoggins sgoggins added deployed version Live problems with deployed versions python Pull requests that update Python code tech debt labels Feb 23, 2026
@sgoggins sgoggins self-assigned this Feb 23, 2026
@MoralCode MoralCode merged commit 1ef058e into main Feb 23, 2026
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployed version Live problems with deployed versions python Pull requests that update Python code tech debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants