Skip to content

fix(datashare-python): align pagination with spec - #68

Merged
ClemDoum merged 2 commits into
mainfrom
fix(datashare-python)/pagination
Aug 5, 2026
Merged

fix(datashare-python): align pagination with spec#68
ClemDoum merged 2 commits into
mainfrom
fix(datashare-python)/pagination

Conversation

@ClemDoum

@ClemDoum ClemDoum commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

solves #67

@ClemDoum
ClemDoum marked this pull request as ready for review August 3, 2026 09:52
@ClemDoum
ClemDoum force-pushed the fix(datashare-python)/pagination branch from 73c01ad to 3974c4b Compare August 3, 2026 09:57
@ClemDoum
ClemDoum requested a review from pirhoo August 3, 2026 09:58
@ClemDoum ClemDoum self-assigned this Aug 3, 2026
@ClemDoum
ClemDoum force-pushed the fix(datashare-python)/pagination branch 2 times, most recently from 0b27664 to 43b1e0f Compare August 3, 2026 10:11

@pirhoo pirhoo 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.

Nice tightening of the pagination shape, and moving total up into a wrapper does read closer to the spec. Two blockers below sit in workers/extract-worker/, which this PR does not touch, so GitHub will not let me anchor them inline. They are the reason I am requesting changes.

workers/extract-worker/extract_worker/activities.py · lines 221-224

crucial: total moved off BasePagination, and icij_config() sets extra="forbid", so this call now raises ValidationError: total / Extra inputs are not permitted. That kills every successful doc in extract_markdown_content_act, and CI runs the extract-worker suite on any datashare-python/** change. Could we wrap it in the new Pages instead (and add Pages to the datashare_python.objects import at the top)?

            pages = extract_res.output.pages
            pages = Pages(
                total=pages.total,
                pagination=ByteRangesPagination(byte_ranges=pages.byte_ranges),
            )

workers/extract-worker/extract_worker/objects.py · line 50

important: paired with the above, the manifest field needs the wrapper type too, otherwise total is silently dropped from manifest.json and entry.pages.total in test_extract_markdown_content_act stops resolving. Swap ByteRangesPagination for Pages in the import as well.

    pages: Pages

Comment thread datashare-python/datashare_python/objects.py Outdated
Comment thread datashare-python/pyproject.toml
Comment thread datashare-python/tests/test_objects.py Outdated
@ClemDoum
ClemDoum force-pushed the fix(datashare-python)/pagination branch from bb8d0e1 to 7378217 Compare August 3, 2026 15:42
@ClemDoum
ClemDoum force-pushed the fix(datashare-python)/pagination branch from 7378217 to 6ea1abf Compare August 3, 2026 15:49
@ClemDoum
ClemDoum requested a review from pirhoo August 3, 2026 15:53

@pirhoo pirhoo 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 for the changes!

@ClemDoum
ClemDoum merged commit 39189cb into main Aug 5, 2026
11 checks passed
@ClemDoum
ClemDoum deleted the fix(datashare-python)/pagination branch August 5, 2026 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants