Ignore PRs once repo is archived #45
Conversation
This matches how we're handling dates in the main CLI invocations.
| if not pr["repo_archived_at"]: | ||
| return True | ||
|
|
||
| if pr[key] < pr["repo_archived_at"]: |
There was a problem hiding this comment.
I don't think this can be right. It's keeping all PRs that were created/merged before the repo was archived, which will be all of them. Don't we need instead to compare the archive date with the "current" date (where "current" scans through time in back-filling)?
There was a problem hiding this comment.
argh, yep, you're completely right
| @@ -69,10 +70,12 @@ def pr_throughput(ctx, org, date): | |||
|
|
|||
| with TimescaleDBWriter(GitHubPullRequests) as writer: | |||
| opened_prs = api.prs_opened_on_date(org, date) | |||
| opened_prs = drop_archived_prs(opened_prs) | |||
There was a problem hiding this comment.
I don't see the need for this filter. There won't be any newly-opened PRs for an archived repo, will there?
| log.info("%s | %s | Processing %s opened PRs", date, org, len(opened_prs)) | ||
| process_prs(writer, opened_prs, date, name="prs_opened") | ||
|
|
||
| merged_prs = api.prs_merged_on_date(org, date) | ||
| merged_prs = drop_archived_prs(merged_prs, key="merged") |
There was a problem hiding this comment.
I don't see the need for this filter. There won't be any newly-merged PRs for an archived repo, will there?
| @@ -40,7 +39,7 @@ source = [ | |||
| ] | |||
|
|
|||
| [tool.coverage.report] | |||
| fail_under = 69 | |||
| fail_under = 67 | |||
There was a problem hiding this comment.
Not sure what the point is of asserting on coverage if we're just going to keep bumping the threshold down. It may not be worth writing tests for full coverage, but in that case we don't get any value from measuring it.
There was a problem hiding this comment.
Yeah, this is the worst part of <100% coverage. My next PR bumps it up to 73 if that helps…?
| assert {o["created"] for o in output} == { | ||
| pr1["created"], | ||
| pr4["created"], | ||
| } |
There was a problem hiding this comment.
This assertion seems a bit more complex than it needs to be. Could it not just read like this?
assert output == {pr1, pr4}
There was a problem hiding this comment.
You can't hash dicts! Otherwise yes I'd love to reduce it down.
|
Closing this one in favour of:
|
This moves the
github backfillcommand to using @benbc's GraphQL queries to pull the repos then PRs for a given org. I took a look at doing this in one query but consuming subpages added loads of complexity, and Ben had already written the other code, so easy win. The goal with moving to the GraphQL API for backfilling was to get the archivedAt value for repos, which isn't present in GitHub's REST API.That allowed the second half of this change, to ignore PRs after a repo has been archived.