GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-143672: convert the struct module to AC by skirpichev · Pull Request #143673 · python/cpython

Conversation

skirpichev commented Jan 11, 2026 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Issue: Convert the struct module to use the Argument Clinic #143672

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          pythongh-143672: convert the struct module to AC

877f65d

bedevere-app Bot mentioned this pull request

Jan 11, 2026

Convert the struct module to use the Argument Clinic #143672

Closed

skirpichev added the skip news label

Jan 11, 2026

skirpichev mentioned this pull request

Jan 11, 2026

gh-78724: raise RuntimeError's when calling methods on non-ready Struct()'s #143643

Merged

skirpichev marked this pull request as ready for review

January 11, 2026 02:49

bedevere-app Bot added the awaiting review label

Jan 11, 2026

sobolevn reviewed

Jan 11, 2026

View reviewed changes

Comment thread

Modules/_struct.c


		See help(struct) for more on format strings.
		Return a new Struct object which writes and reads binary data according
		to the format string. See help(struct) for more on format strings.

sobolevn Jan 11, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

what is the reason for newline -> double space change? I was not aware that this is required.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

skirpichev Jan 11, 2026

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

In some docstrings "See help(struct) for more on format strings." was separated to a new paragraph. In some - no. Looks as a waste of space, especially in help(struct) output.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_struct.c Outdated

		/
		*args: array

		Pack args to the writtable buffer according to the format self.

sobolevn Jan 11, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	Pack args to the writtable buffer according to the format self.
	Pack args to the writtable buffer according to the struct's format string.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

skirpichev Jan 11, 2026

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Too long. Beware @permit_long_summary.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_struct.c Outdated

		strings.");
		*args: array

		Return a bytes object with args, packed according the format self.

sobolevn Jan 11, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	Return a bytes object with args, packed according the format self.
	Return a bytes object with args, packed according the struct's format string.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

skirpichev commented Jan 11, 2026

Copy link

Copy Markdown

Member Author

BTW, I'm not attached to docstrings.

IMO, they aren't good and I tried to improve that. But I can just revert that stuff and keep old doctsrings almost literally (summary line is required, though).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

skirpichev and others added 8 commits

January 12, 2026 02:24


          Merge branch 'main' into struct-AC/143672

873be65

+1

5716dba


          Merge branch 'master' into struct-AC/143672

905e545


          Change Struct's docstring & add test in test_inspect.py

df0f7c4

Co-authored-by: Serhiy Storchaka <[email protected]>


          + simplify s_pack_internal()

76907cf


          Merge branch 'main' into struct-AC/143672

c0d7480


          args -> values and request a writable buffer

5e4947a


          + tests for read-only buffers

4063a99

serhiy-storchaka reviewed

Jan 15, 2026

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry, I have not noticed this PR and created an alternative PR #143857.

There are some differences between two PRs, and I had reasons for all changes. We can now gradually convert your PR into my PR by applying one change at a time or just take my PR.

See the difference between the PRs: https://github.com/skirpichev/cpython/pull/14/changes .

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_struct.c

		Struct.pack_into as s_pack_into

		buffer: Py_buffer
		offset: Py_ssize_t

serhiy-storchaka Jan 15, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

There is a subtle behavior difference here (IndexError vs OverflowError). I leave this for other issue.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_struct.c Outdated

		buffer: Py_buffer
		offset: Py_ssize_t
		/
		*args: array

serhiy-storchaka Jan 15, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I prever to use more specific name values.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_struct.c Outdated

		/*[clinic input]
		Struct.pack_into as s_pack_into

		buffer: Py_buffer

serhiy-storchaka Jan 15, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The buffer must be writeable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_struct.c

		offset. Note that the offset is a required argument. See\n\
		help(struct) for more on format strings.");
		/*[clinic input]
		Struct.pack_into as s_pack_into

serhiy-storchaka Jan 15, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

No need to rename it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

skirpichev mentioned this pull request

Jan 15, 2026

gh-143672: Finish conversion of the struct module to Argument Clinic #143857

Merged

skirpichev closed this

Jan 15, 2026

skirpichev deleted the struct-AC/143672 branch

January 15, 2026 12:36

Reviewers

serhiy-storchaka

serhiy-storchaka left review comments

sobolevn

sobolevn left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

skirpichev commented Jan 11, 2026 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skirpichev commented Jan 11, 2026

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

skirpichev commented Jan 11, 2026 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.