Skip to content

Mark positional-only parameters in the IcePy stub and docstrings - #6607

Merged
pepone merged 4 commits into
mainfrom
fix/icepy-stub-positional-only
Aug 14, 2026
Merged

Mark positional-only parameters in the IcePy stub and docstrings#6607
pepone merged 4 commits into
mainfrom
fix/icepy-stub-positional-only

Conversation

@pepone

@pepone pepone commented Aug 14, 2026

Copy link
Copy Markdown
Member

The IcePy stub declared ordinary, keyword-capable parameters for almost every function, but the C functions behind them are registered METH_VARARGS (or METH_O) and parse with PyArg_ParseTuple, so they accept positional arguments only. Type checkers therefore accepted calls such as connection.setAdapter(adapter=None) that raise TypeError at runtime. The constructors are worse: their tp_init ignores the keyword dict, so IcePy.Properties(args=[...]) silently dropped the argument.

This PR adds a / marker to every affected declaration in the stub, and the same marker to the signature line of the corresponding IcePy docstrings, which must match the stub (checkIcePyStub.py) and feed the Sphinx API reference. ice_invoke and ice_invokeAsync are registered with METH_KEYWORDS and genuinely accept keyword arguments; they are unchanged.

Fixes #6442

The IcePy stub declared ordinary, keyword-capable parameters for almost
every function, but the C functions behind them are registered
METH_VARARGS (or METH_O) and parse with PyArg_ParseTuple, so they accept
positional arguments only. Type checkers therefore accepted calls such
as connection.setAdapter(adapter=None) that raise TypeError at runtime.
The constructors are worse: their tp_init ignores the keyword dict, so
IcePy.Properties(args=[...]) silently dropped the argument.

Add a '/' marker to every affected declaration in the stub, and add the
same marker to the signature line of the corresponding IcePy docstrings,
which must match the stub (checkIcePyStub.py) and feed the Sphinx API
reference. ice_invoke and ice_invokeAsync are registered with
METH_KEYWORDS and genuinely accept keyword arguments; they are
unchanged.

Fixes #6442
@pepone pepone added this to the 3.8.3 milestone Aug 14, 2026
@pepone pepone added the python label Aug 14, 2026
Copilot AI balanced review requested due to automatic review settings August 14, 2026 09:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Aligns IcePy type stubs and documentation with the extension’s positional-only runtime APIs.

Changes:

  • Marks affected parameters positional-only in the IcePy stub.
  • Updates corresponding C-extension docstrings.
  • Adds the #6442 changelog fragment.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file
File Description
python/python/IcePy-stubs/__init__.pyi Corrects positional-only signatures.
python/modules/IcePy/Proxy.cpp Updates proxy method signatures.
python/modules/IcePy/PropertiesAdmin.cpp Updates properties-admin signatures.
python/modules/IcePy/Properties.cpp Updates properties signatures.
python/modules/IcePy/Operation.cpp Updates operation signatures.
python/modules/IcePy/ObjectAdapter.cpp Updates adapter signatures.
python/modules/IcePy/Logger.cpp Updates logger signatures.
python/modules/IcePy/Init.cpp Updates module function signatures.
python/modules/IcePy/ImplicitContext.cpp Updates context signatures.
python/modules/IcePy/Connection.cpp Updates connection signatures.
python/modules/IcePy/Communicator.cpp Updates communicator signatures.
changelog.d/python/6442.md Documents the correction.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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

Looks good.

  • 24 rich-comparison dunders still declare a named parameter — Connection (:313-319), Endpoint (:372-377), ImplicitContext (:463-468), ObjectPrx (:600-606). They're tp_richcompare slot wrappers, so __eq__(other=1) raises TypeError: takes no keyword arguments exactly like the methods this PR fixes. Cosmetic, since widening object.__eq__ is legal against typeshed — but isDunder in checkIcePyStub.py exempts them, so nothing will catch the drift later.
  • Heads-up on merge order with #6609: it rewrites the same module-level function block (createProperties, stringToIdentity, identityToString, setProcessLogger, loadSlice, compileSlice) from a base without the /, so a textual conflict is certain. If that side wins the resolution, the stub loses six markers while the C docstrings keep theirs, and the checker #6609 adds fails CI with six signature mismatches.

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

The debug on ubuntu-24.04 failure is in DataStorm/events — unrelated to this PR.

Comment thread changelog.d/python/6442.md Outdated
pepone and others added 2 commits August 14, 2026 16:37
Co-authored-by: Bernard Normier <bernard@zeroc.com>
The rich comparisons on Connection, Endpoint, ImplicitContext, and
ObjectPrx are tp_richcompare slot wrappers, so __eq__(other=1) raises
TypeError just like the METH_VARARGS methods. This also matches
typeshed, which declares object.__eq__ positional-only.
@pepone

pepone commented Aug 14, 2026

Copy link
Copy Markdown
Member Author

🤖 Claude: @externl Both points addressed:

  • The 24 rich-comparison dunders are now marked positional-only too (1c93005) — confirmed at runtime that the tp_richcompare slot wrappers raise TypeError: wrapper __eq__() takes no keyword arguments, and this matches typeshed, which declares object.__eq__ positional-only. checkIcePyStub.py and the full pyright run stay green.
  • Noted on Close the gaps in the IcePy stub/module consistency check #6609 merge order: whichever PR lands second will rebase the module-level function block keeping both the / markers and the copied prose.

Comment thread changelog.d/python/6442.md Outdated

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.

Since this is all for IcePy I don't think it deserves a changelog entry.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

removed

@pepone
pepone merged commit d7e9cf5 into main Aug 14, 2026
34 checks passed
@pepone
pepone deleted the fix/icepy-stub-positional-only branch August 14, 2026 15:33
externl pushed a commit that referenced this pull request Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: the IcePy stub declares keyword-capable parameters for functions that accept positional arguments only

4 participants