What
tools/bitbucket/src/magpie_bitbucket/datacenter.py repeats the same non-advancing-pagination guard in six functions:
if next_start <= start:
break
get_repository_restrictionslist_open_pull_requestsget_pull_request_commitsget_pull_request_statusget_pull_request_reviewsget_pull_request_discussion
cloud.py has the equivalent protection — a server that keeps handing back the same page — but centralised in one _validated_next_url() helper that every paginating call routes through.
Why it matters
The duplication is not just untidy; it means each copy needs its own test, and they do not all have one.
#1047 added a test pinning the guard in get_pull_request_commits, which is good. While reviewing it I deleted the same guard from get_repository_restrictions instead, by mistake — and the entire 101-test suite still passed. So at least one copy is completely unpinned: it can be removed and nothing notices.
The failure it protects against is an infinite request loop against a remote server, which is the kind of bug you want a test to catch rather than a user.
The contrast with cloud.py is the useful part. Because cloud's guard lives in one helper, a single test covers every caller, and a newly added paginating endpoint inherits the protection for free — as #1041 demonstrated when it added get_pull_request_tasks and got the repeated-URL check without doing anything.
Suggested fix
Hoist the datacenter guard into a shared helper the way cloud.py does — something that owns the "advance start, or stop" decision and is called by all six paginators. That would:
- make one test cover all six call sites, rather than needing five more tests;
- mean the next Data Center paginator cannot be written without the guard;
- bring the two backends' pagination handling into the same shape, which makes them easier to compare when either changes.
Worth doing as its own change rather than folded into a feature PR, since it touches six functions.
What
tools/bitbucket/src/magpie_bitbucket/datacenter.pyrepeats the same non-advancing-pagination guard in six functions:get_repository_restrictionslist_open_pull_requestsget_pull_request_commitsget_pull_request_statusget_pull_request_reviewsget_pull_request_discussioncloud.pyhas the equivalent protection — a server that keeps handing back the same page — but centralised in one_validated_next_url()helper that every paginating call routes through.Why it matters
The duplication is not just untidy; it means each copy needs its own test, and they do not all have one.
#1047 added a test pinning the guard in
get_pull_request_commits, which is good. While reviewing it I deleted the same guard fromget_repository_restrictionsinstead, by mistake — and the entire 101-test suite still passed. So at least one copy is completely unpinned: it can be removed and nothing notices.The failure it protects against is an infinite request loop against a remote server, which is the kind of bug you want a test to catch rather than a user.
The contrast with
cloud.pyis the useful part. Because cloud's guard lives in one helper, a single test covers every caller, and a newly added paginating endpoint inherits the protection for free — as #1041 demonstrated when it addedget_pull_request_tasksand got the repeated-URL check without doing anything.Suggested fix
Hoist the datacenter guard into a shared helper the way
cloud.pydoes — something that owns the "advancestart, or stop" decision and is called by all six paginators. That would:Worth doing as its own change rather than folded into a feature PR, since it touches six functions.