Skip to content

fix(tools/gitlab): address review follow-up on pagination docstring, short pages, and issue template - #1441

Merged
potiuk merged 2 commits into
apache:mainfrom
Kaap10:fix/gitlab-review-followup
Sep 28, 2026
Merged

potiuk merged 2 commits into
apache:mainfrom
Kaap10:fix/gitlab-review-followup

Conversation

@Kaap10

@Kaap10 Kaap10 commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Addresses the two non-blocking follow-up observations from maintainer review in PR #1369:

1. tools/gitlab/issue-template.md

  • Moved the SPDX-License-Identifier header above the doctoc Table of Contents block to conform to standard ASF file layout across the repository.
  • Reworded the closing sentence to clarify that it serves as a descriptive reference for Markdown constructs in raw JSON issue/MR payloads.

2. tools/gitlab/src/magpie_gitlab/client.py & tools/gitlab/tests/test_client.py

  • Corrected the test_get_paged_json_with_limit_fewer_than_page docstring from stating that it "emits notice" to "emits no notice", matching the actual assertion (err == "").
  • Refined the pagination loop in get_paged_json to continue fetching subsequent pages when explicit --limit is specified and a short page (<100 items) is encountered with X-Next-Page set.
  • Removed unused math import.
  • Added test_get_paged_json_short_page_continues_until_limit to verify multi-page collection across short pages.

All 61 unit tests, mypy static typing, and ruff checks pass cleanly.

@github-actions github-actions Bot added contract:tracker Tool capability: issue / board / label backend contract:source-control Tool capability: branch / commit / diff / push (VCS) contract:change-request Tool capability: proposed-change review + merge gate (PR / MR / Gerrit change) labels Sep 28, 2026
@Kaap10

Kaap10 commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

Hi @potiuk, opening this quick follow-up PR to address the two smaller observations from #1369.
Ready for review whenever you get a chance!

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks — both follow-ups from #1369 are closed: with an explicit --limit the loop now keeps paging through short pages until the limit or the last page, max_pages only caps unlimited fetches (where the notice's "use --limit" remedy actually works), and the new test covers it. The docstring, SPDX placement and closing sentence are fixed too.

I pushed one small fixup on top (7eabef3): the change had also dropped the *generated with [DocToc](…)* suffix from the TOC title that every other doc keeps, so I restored it. Merging.


This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.

More on how Apache Magpie handles maintainer review:
Contributing guide.

@potiuk potiuk added the family:tools tools/* label Sep 28, 2026
@potiuk
potiuk merged commit 3d0f492 into apache:main Sep 28, 2026
9 checks passed
@potiuk potiuk mentioned this pull request Sep 28, 2026
4 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contract:change-request Tool capability: proposed-change review + merge gate (PR / MR / Gerrit change) contract:source-control Tool capability: branch / commit / diff / push (VCS) contract:tracker Tool capability: issue / board / label backend family:tools tools/*

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants