Repository navigation
Dead code and tuple bug in from_github() #178
Description
Note
Migrated from augurlabs/augur#3596
Originally opened by @iGufrankhan on 2026-01-16
Description:
While reviewing PR ingestion, I found that from_github() assigns many fields with a trailing comma, which makes them 1-element tuples instead of scalar values.
for example :
pr_obj.pr_src_id = int(str(pr['id']).encode(encoding='UTF-8').decode(encoding='UTF-8')),
pr_obj.pr_src_node_id = pr['node_id'],
pr_obj.pr_html_url = pr['html_url'],
pr_obj.pr_diff_url = pr['diff_url'],
pr_obj.pr_patch_url = pr['patch_url'],
pr_obj.pr_issue_url = pr['issue_url'],
file location :
https://github.com/chaoss/augur/blob/main/augur/application/db/models/augur_data.py
line : 1805
This syntax creates tuples like (value,) instead of value,
This is almost certainly unintended.
However, after tracing ingestion flow, it seems that PR ingestion now uses:
extract_data_from_pr_list(...)
So this buggy function may currently be dead / unused code, which is why the tuple bug has not caused visible failures.
#question is if it is dead why this matter:
1.railing commas silently create tuples → dangerous hidden bug
2.Function appears unused → dead code increases confusion and maintenance cost
3.Future refactors might accidentally re-enable this broken path
One of the following should happen:
Option A: Fix tuple bug by removing trailing commas
Option B: Remove or deprecate from_github() if it is fully unused
Before opening a PR, I want to confirm:
Is PullRequest.from_github() completely deprecated in the current ingestion pipeline?
If yes ,then remove the bug and add a simple comment that this complety dead
if no ,then only remove the trailing comma bug
Activity
collectoss-issue-migrator commentedon Apr 30, 2026
Note
Migrated comment from original
Originally posted by
@guptapratykshhon 2026-01-17i think by time , you can remove trailing commas and document method as dead code superseded by extract_data_from_pr_list
collectoss-issue-migrator commentedon Apr 30, 2026
Note
Migrated comment from original
Originally posted by
@MoralCodeon 2026-01-17if we can confirm this code is actually dead, id say we should just remove it entirely to debloat things. Unless maybe theres still a use for it (maybe it was a planned future improvement we never finished switching to).
might take some diving into the code and git history to find out what the story is here.
collectoss-issue-migrator commentedon Apr 30, 2026
Note
Migrated comment from original
Originally posted by
@iGufrankhanon 2026-01-18ok let me confirm this ,
if code is dead and there is no use in future also then i go ahead and remove it completely
else i will fill it tailling comma error and make a pr
before that let me verify all condition
collectoss-issue-migrator commentedon Apr 30, 2026
Note
Migrated comment from original
Originally posted by
@shlokgildaon 2026-02-01Did some digging to confirm the status of this code:
PullRequest.from_github()is fully dead. Zero callers anywhere in the codebase. The current PR ingestion pipeline goes throughextract_needed_pr_data()in https://github.com/chaoss/augur/blob/main/augur/application/db/data_parse.py#L506, which returns plain dicts for bulk insert. Thefrom_github()path was replaced a while ago and never cleaned up.So yeah, just remove the method entirely. No need to fix the trailing commas on dead code. @iGufrankhan
Worth noting: there are ~9 other
from_github()methods on other model classes (Contributor, IssueLabel, PullRequestLabel, etc.) that are also unused. Those could be a follow-up cleanup if anyone wants to tackle it, but keeping this PR scoped to just thePullRequestone makes sense.collectoss-issue-migrator commentedon Apr 30, 2026
Note
Migrated comment from original
Originally posted by
@iGufrankhanon 2026-02-01@shlokgilda Thanks for confirming
Since PullRequest.from_github() has zero callers and the PR ingestion pipeline fully uses extract_needed_pr_data(),
should I remove this method entirely,
or
would you prefer it to be commented out with a note that it’s dead/unused?
also I’m currently tied up with college exams, but I can open the PR shortly after they finish.
If that timing doesn’t work, feel free to assign this cleanup to someone else.
collectoss-issue-migrator commentedon Apr 30, 2026
Note
Migrated comment from original
Originally posted by
@shlokgildaon 2026-02-01I think it's fine. This isn't a high priority fix. So feel free to open up the PR whenever you can. And I think we can go ahead and remove the dead code.
collectoss-issue-migrator commentedon Apr 30, 2026
Note
Migrated comment from original
Originally posted by
@MoralCodeon 2026-02-07in reviewing the linked PR, I noticed there are a TON of other from_github methods that @ABrain7710 added a while ago and seemingly havent used.
I might actually refactor these into a github specific interface wrapper as part of #176
collectoss-issue-migrator commentedon Apr 30, 2026
Note
Migrated comment from original
Originally posted by
@MoralCodeon 2026-03-04if we are going to have any kind of "from" interface in our DB models, it should be an interface adapting from our own internal model of a forge user, or things like that - providing an intermediary type that every different forge can adapt to without adding code in our models for every single forge we support