Skip to content

Start consolodating Github REST API URLs into a central place#3685

Open
MoralCode wants to merge 13 commits intomainfrom
ntdn/consolodate_github_rest_urls1
Open

Start consolodating Github REST API URLs into a central place#3685
MoralCode wants to merge 13 commits intomainfrom
ntdn/consolodate_github_rest_urls1

Conversation

@MoralCode
Copy link
Copy Markdown
Collaborator

@MoralCode MoralCode commented Feb 12, 2026

Description
This takes inspiration from the discussion in #3597 and is a step towards resolving #3277

This pr:

  • refactors a few commonly reused functions that generate github API URLs (specifically for the issues, contributors, and user endpoints) so that the url building happens within the existing GithubDataAccess class.
  • creates a function that brings the fetching of github user API data into this class as well (a model for future plans for the other endpoints)
  • creates a user_endpoint_urls function that, given a username, can recreate the urls that the github API responds with. This change compliments Remove API url columns (that can be recalculated) from the contributors table #3747.
  • creates a currently-unused general-purpose function to generate URLs for querying Github's search API, removing the need to manually assemble the URL in an API-compliant way from clients that use it. clients just provide the query string as it would be typed into githubs search box. This change also enabled some current legacy facade code for doing these searches to be replaced with this general-purpose implementation.

Notes for Reviewers
Augur collection appears to run smoothly with this PR on my local dev instance, indicating this change doesnt cause breakage.

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.

Like the idea.

One thing that needs fixing before merge: there's an operator precedence bug in issues_endpoint_url that'll break the trailing_slash=False path. Python evaluates the ternary after the +, so when trailing_slash=False you get an empty string back instead of the URL without a slash. The issues endpoint actually hits this path, so it'd break issue collection.

Also, _base_url() is defined but never called. issues_endpoint_url still hardcodes the domain. Worth wiring that in since that's the whole point of the refactor.

Details in the inline comments.

shlokgilda
shlokgilda previously approved these changes Feb 13, 2026
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.

LGTM

@sgoggins
Copy link
Copy Markdown
Collaborator

Needs to be tested before merge.

@MoralCode MoralCode added the testing Related to Augur's testing suite label Feb 13, 2026
@MoralCode MoralCode requested a review from sgoggins as a code owner March 5, 2026 00:29
github_data_access = GithubDataAccess(key_auth, logger)

# Set the base of the url and place to hold contributors to insert
contributors_url = github_data_access.contributors_endpoint_url(owner, repo) + "?state=all"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'repo' (undefined-variable)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this commit shouldnt be here....

@MoralCode MoralCode force-pushed the ntdn/consolodate_github_rest_urls1 branch from e831bc5 to 5b6e5f8 Compare March 16, 2026 20:06
@MoralCode
Copy link
Copy Markdown
Collaborator Author

currently testing on my dev instance

@MoralCode MoralCode added ready Items tested and seeking additional approvals or a merge. Usually for items under active development and removed testing Related to Augur's testing suite labels Mar 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready Items tested and seeking additional approvals or a merge. Usually for items under active development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants