GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-126615: Make `COMError` public and add to `ctypes` doc. by junkmd · Pull Request #126686 · python/cpython

Conversation

@junkmd

@junkmd junkmd commented Nov 11, 2024 •
edited by github-actions Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Please refer also to gh-126384 and gh-126610 for the behavior when a foreign function fails to call a COM method.

  • Issue: ctypes: Make COMError, mentioned in the "Function prototypes" section of documentation, public. #126615

📚 Documentation preview 📚: https://cpython-previews--126686.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-app bedevere-app Bot added docs Documentation in the Doc dir skip news awaiting review labels Nov 11, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Nov 11, 2024
ctypes: Make COMError, mentioned in the "Function prototypes" section of documentation, public. #126615
Closed
@ZeroIntensity ZeroIntensity added topic-ctypes needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels Nov 11, 2024
junkmd added 4 commits November 16, 2024 09:18
@junkmd
add COMError exception to Doc/library/ctypes.rst.
e70aa54
@junkmd
Add a mention of the difference in exceptions that can be raised.
181f512
@junkmd
Add a dot to refer to the object within the same module.
857fef5
@junkmd
Simplify descriptions of attributes.
90fb47f
@junkmd
junkmd force-pushed the gh_126615_comerror_doc branch from 58cc032 to 90fb47f Compare November 16, 2024 09:22
@junkmd junkmd mentioned this pull request Nov 16, 2024
Methods to bridge the gap between Integer error codes and HRESULT values represented as signed 32-bit hexadecimal enthought/comtypes#665
Closed
@junkmd
Align with the docstring defined in the C implementation:
1f9d4ee
"Raised when a COM method call failed."

https://github.com/python/cpython/blob/2313f8421080ceb3343c6f5d291279adea85e073/Modules/_ctypes/_ctypes.c#L5548
picnixz
picnixz reviewed Nov 16, 2024
View reviewed changes
Comment thread Doc/library/ctypes.rst Resolved

Uh oh!

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

Comment thread Doc/library/ctypes.rst Outdated

.. exception:: COMError(hresult, text, details)

Windows only: This non-public exception is raised when a COM method call

@picnixz picnixz Nov 16, 2024

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.

You can use .. availability:: Windows (but put the directive after the class doc)

Sorry, something went wrong.

Uh oh!

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

junkmd reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@junkmd junkmd Nov 16, 2024

Copy link
Copy Markdown
Contributor 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.

I followed the pattern in ctypes.rst because it frequently used sentences like Windows only: ....

Since this markup wasn't mentioned in the documentation guide, I overlooked it.
Is this a more modern approach?

Sorry, something went wrong.

Uh oh!

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

All reactions

@picnixz picnixz Nov 16, 2024 •
edited
Loading

Uh oh!

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

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.

It's widely used in the https://docs.python.org/3/library/socket.html module actually. But if it's not the pattern of ctypes maybe it's better to be consistent. We can make a follow-up PR to transform them into directives.

Sorry, something went wrong.

Uh oh!

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

junkmd reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@junkmd junkmd Nov 16, 2024

Copy link
Copy Markdown
Contributor 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.

I removed the Availability directive and reverted to the original "Windows only: ...".

Once this PR is merged, I would like to work for replacing "Foo only: ..." with the .. availability:: Foo directive.

Sorry, something went wrong.

Uh oh!

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

picnixz reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@junkmd junkmd Nov 17, 2024

Copy link
Copy Markdown
Contributor 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.

Referring to the socket.ioctl documentation, it seems that using :platform: Windows might be more appropriate than .. availability:: Windows. What do you think?

Alternatively, could it be that :platform: is intended to be used exclusively in module sections, as described in the documentation guide's "Module-specific markup"?

Sorry, something went wrong.

Uh oh!

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

All reactions

@encukou encukou Nov 18, 2024

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.

.. availability:: is generally the correct one to use. FWIW, we have some tooling to ensure it's in a correct format.
It's OK to not use it in this PR, to be consistent with the rest of the documentation. But, if you're looking for more to do in the ctypes docs, adding availability would make a great follow-up PR :)

Sorry, something went wrong.

Uh oh!

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

junkmd reacted with heart emoji
All reactions
  • ❤️ 1 reaction
Comment thread Doc/library/ctypes.rst Resolved

Uh oh!

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

Comment thread Doc/library/ctypes.rst Outdated
are specified in the :attr:`!argtypes` tuple.


.. exception:: COMError(hresult, text, details)

@picnixz picnixz Nov 16, 2024

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.

Should it be placed here or somewhere else? because it cuts the flow of the reading where paramflags is documented afterwards. Maybe we can put it before the functions?

Sorry, something went wrong.

Uh oh!

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

junkmd reacted with eyes emoji
All reactions
  • 👀 1 reaction

@junkmd junkmd Nov 16, 2024

Copy link
Copy Markdown
Contributor 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.

I was also unsure whether this was the right place to put it.

On the other hand, placing it above prototype or above FOOFUNCTYPE might seem abrupt.

Might it be appropriate to create the Exceptions section, including ArgumentError?
However, in this scope, I think making such a change that involves the "Foreign functions" section is excessive.

Sorry, something went wrong.

Uh oh!

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

All reactions

@picnixz picnixz Nov 16, 2024

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.

Well.. honestly I think it makes sense to put it in an exception section. I think it could be fine for this small docs change (I mean, it's for our own convenience). We can ask @hugovk as a docs expert.

Sorry, something went wrong.

Uh oh!

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

junkmd reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@hugovk hugovk Nov 16, 2024

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.

Yes, we often have an exceptions section further down, see for example:

https://docs.python.org/3/library/json.html#exceptions

Sorry, something went wrong.

Uh oh!

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

junkmd reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@junkmd junkmd Nov 16, 2024

Copy link
Copy Markdown
Contributor 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.

If the community agrees, creating an "Exceptions" section is no problem at all.

For now, I think I'll try placing it at the end of the document, after the "Arrays and pointers" section.

Sorry, something went wrong.

Uh oh!

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

encukou reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@picnixz
picnixz requested a review from encukou November 16, 2024 11:42
@junkmd @picnixz
Apply suggestions from code review
b71e310
Co-authored-by: Bénédikt Tran <[email protected]>
picnixz
picnixz reviewed Nov 16, 2024
View reviewed changes
Comment thread Doc/library/ctypes.rst Resolved

Uh oh!

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

junkmd added 2 commits November 16, 2024 12:21
@junkmd
Add the dot to make the reflink.
dafb5b2
@junkmd
Add the availability.
49d5e5f
@junkmd
junkmd requested a review from picnixz November 16, 2024 12:55
junkmd added 2 commits November 16, 2024 13:38
@junkmd
Create the "Exceptions" section.
1fa159a
@junkmd
Remove the availability directive
934c36a
in order to maintain consistency with other sections.
@encukou

encukou commented Nov 18, 2024

Copy link
Copy Markdown
Member

AFAICS, COMError is available in _ctypes, but not ctypes. So, documenting it as ctypes.COMError would be misleading.
I think we should re-export it from ctypes -- it's not very usable otherwise.

All reactions

Sorry, something went wrong.

Uh oh!

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

encukou
encukou reviewed Nov 18, 2024
View reviewed changes
Comment thread Doc/library/ctypes.rst Outdated

.. attribute:: details

The 5-tuple representing additional details about the error.

@encukou encukou Nov 18, 2024

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.

That's not very helpful. Could you document what the items are?

Sorry, something went wrong.

Uh oh!

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

All reactions

@junkmd junkmd Nov 18, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor 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.

Based on the C implementation source code, these items are retrieved via methods of IErrorInfo1: GetDescription, GetSource, GetHelpFile, GetHelpContext, and the ProgID obtained via using ProgIDFromCLSID and GetGUID.

However, this exception can be raised not only from foreign functions but also intentionally by passing variable-length tuples, as seen in test_win32.py and the comtypes package.

cpython/Lib/test/test_ctypes/test_win32.py

Lines 67 to 76 in f9c5573

def test_COMError(self):
from _ctypes import COMError
if support.HAVE_DOCSTRINGS:
self.assertEqual(COMError.__doc__,
"Raised when a COM method call failed.")
ex = COMError(-1, "text", ("details",))
self.assertEqual(ex.hresult, -1)
self.assertEqual(ex.text, "text")
self.assertEqual(ex.details, ("details",))

I will revise this part, taking these facts into account.

Footnotes

  1. Reference: IErrorInfo implementation in comtypes. ↩

Sorry, something went wrong.

Uh oh!

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

All reactions

@encukou encukou Nov 18, 2024

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.

Since _ctypes is internal API that is subject to change, I think it would be best to change test_win32 and comtypes so that they raise the same 5-tuple as ctypes itself, and document that 5-tuple.

Sorry, something went wrong.

Uh oh!

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

All reactions

@junkmd junkmd Nov 18, 2024

Copy link
Copy Markdown
Contributor 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.

I wrote details of details.

I plan to improve test_win32 after improving this documentation.

For comtypes, I will investigate how much COMError.details is being used and then discuss the matter with the project's community to decide how to proceed.

As mentioned earlier, I would like to focus on improving the documentation in this PR.

Sorry, something went wrong.

Uh oh!

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

All reactions
encukou
encukou reviewed Nov 18, 2024
View reviewed changes
Comment thread Doc/library/ctypes.rst Outdated
integer. *name* is name of the COM method. *iid* is an optional pointer to
the interface identifier which is used in extended error reporting.

If *iid* is not specified, a :exc:`WindowsError` is raised if the COM method

@encukou encukou Nov 18, 2024

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.

WindowsError is now an alias of OSError.

Suggested change
If *iid* is not specified, a :exc:`WindowsError` is raised if the COM method
If *iid* is not specified, an :exc:`OSError` is raised if the COM method

Sorry, something went wrong.

Uh oh!

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

junkmd reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@junkmd junkmd Nov 18, 2024

Copy link
Copy Markdown
Contributor 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.

Good catch.

Thank you.

Sorry, something went wrong.

Uh oh!

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

All reactions
@junkmd

junkmd commented Nov 18, 2024

Copy link
Copy Markdown
Contributor Author

AFAICS, COMError is available in _ctypes, but not ctypes. So, documenting it as ctypes.COMError would be misleading.
I think we should re-export it from ctypes -- it's not very usable otherwise.

_CData cannot be imported from either _ctypes or ctypes, but it is documented as "non-public". I consider COMError to be something similar.

I DO agree that COMError has value as a public exception.
However, for now, I would like to separate scopes.

In this PR, I will focus solely on improving the documentation, and I plan to address the new tasks that arise here by opening separate issues.

All reactions

Sorry, something went wrong.

Uh oh!

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

junkmd added 2 commits November 18, 2024 15:04
@junkmd
WindowsError -> OSError
9461642
@junkmd
details of details.
705ffcc
@junkmd
junkmd requested a review from encukou November 18, 2024 15:14
@encukou

encukou commented Nov 19, 2024

Copy link
Copy Markdown
Member

I see where you're coming from, but I don't think this PR should be merged as-is.

It's true that ctypes._CData does not exist as documented. This has been the case since before Python 3. I think that's a bug in the documentation¹⁾; it should not be taken as a precedent.

While _CData is private API, but its documentation is useful since it describes public classes that inherit from _CData.

But for _ctypes.COMError, I don't see a way to use it without importing it. If you can't use it with except (or isinstance), knowing its attributes is not very helpful. AFAIK, documenting it would only encourage people to use private API.

So, I think it needs to become public at the same time as it's documented.


¹⁾ Tangentially: I don't think we have a good way to document private base classes. As with COMType, it would probably be easier to make _CData public than to document it properly as is.

junkmd reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

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

@junkmd

junkmd commented Nov 19, 2024

Copy link
Copy Markdown
Contributor Author

Understood.

If the maintainers approve making such a private API public, I have no strong objections.

I initially aimed to update the documentation first because I thought proposals to change production code would require extensive discussions from various perspectives and take longer to merge.

I’ll add from _ctypes import COMError line and the platform bridge to ctypes/__init__.py.

Since this is no longer an internal change, I’ll also add a NEWS entry.

Additionally, I’ll include a .. versionchanged:: directive to indicate that while this has existed as a private API in _ctypes, it became public starting with Python 3.14.

All reactions

Sorry, something went wrong.

Uh oh!

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

@junkmd junkmd changed the title gh-126615: Add COMError to ctypes doc. gh-126615: Make COMError public and add to ctypes doc. Nov 19, 2024
junkmd added 3 commits November 19, 2024 15:19
@junkmd
Make COMError public.
a16f1e2
@junkmd
Update docs.
e2617f3
@junkmd
Update test_COMError.
57d76c5
encukou
encukou reviewed Nov 20, 2024
View reviewed changes

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

Thank you!
I do think this is a better way to do it -- if we want to talk about something in the docs, it's better if it has a public name.

Two more nitpicks:

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/library/ctypes.rst Outdated
Comment on lines +2777 to +2780
.. versionchanged:: 3.14

This exception is now public.
Previously, this was private and only available in ``_ctypes``.

@encukou encukou Nov 20, 2024

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.

Let's not expose private API in the docs.

Suggested change
.. versionchanged:: 3.14
This exception is now public.
Previously, this was private and only available in ``_ctypes``.
.. versionadded:: next

Sorry, something went wrong.

Uh oh!

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

All reactions

@junkmd junkmd Nov 20, 2024

Copy link
Copy Markdown
Contributor 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.

You’re right, this information isn’t particularly useful in the documentation.

I’ll move it to the NEWS file as a supplement.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/library/ctypes.rst Outdated
the interface identifier which is used in extended error reporting.

If *iid* is not specified, an :exc:`OSError` is raised if the COM method
call fails. If *iid* is specified, a :exc:`.COMError` is raised instead.

@encukou encukou Nov 20, 2024

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 think (but I'm not 100% sure) that .COMError means Sphinx will search all namespaces, not just ctypes, for a COMError. So it would be better to use ctypes.COMError explicitly:

Suggested change
call fails. If *iid* is specified, a :exc:`.COMError` is raised instead.
call fails. If *iid* is specified, a :exc:`~ctypes.COMError` is raised instead.

Sorry, something went wrong.

Uh oh!

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

junkmd reacted with eyes emoji
All reactions
  • 👀 1 reaction
junkmd added 4 commits November 20, 2024 12:18
@junkmd
Update the NEWS.
36f1607
@junkmd
.. versionchanged:: -> .. versionadded::
068ea2a
@junkmd
Update the prototype section.
3da9e15
@junkmd
Merge branch 'main' into gh_126615_comerror_doc
920bccc
@junkmd
junkmd requested a review from encukou November 20, 2024 12:29
@encukou
encukou enabled auto-merge (squash) November 20, 2024 12:36
@encukou encukou removed needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes docs Documentation in the Doc dir labels Nov 20, 2024
encukou
encukou approved these changes Nov 20, 2024
View reviewed changes

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

Thank you!

Sorry, something went wrong.

Uh oh!

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

junkmd reacted with rocket emoji
All reactions
  • 🚀 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Nov 20, 2024
@encukou
encukou merged commit 5b45025 into python:main Nov 20, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Nov 20, 2024
@junkmd
junkmd deleted the gh_126615_comerror_doc branch November 20, 2024 13:19
This was referenced Nov 20, 2024
gh-126384: Add tests to verify the behavior of basic COM methods. #126610
Merged
Replace :platform: with .. availability:: in socket.ioctl doc. #127122
Merged
gh-127082: Replace "Windows only: ..." with the .. availability:: Windows directive in ctypes doc. #127099
Merged
Add tests to verify the behavior of _ctypes.CopyComPointer. #127183
Closed
gh-127183: Add _ctypes.CopyComPointer tests #127184
Merged
Make CopyComPointer public and add to ctypes doc. #127255
Closed
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@junkmd @picnixz @ebonnal
pythongh-126615: ctypes: Make COMError public (pythonGH-126686)
1e1f7a4
Co-authored-by: Bénédikt Tran <[email protected]>
@coderabbitai coderabbitai Bot mentioned this pull request Jul 25, 2026
[codex] 프로세스 직접 실행 인자 전달 추가 lsh930309/HomeworkHelperServer#39
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hugovk hugovk hugovk left review comments
@encukou encukou encukou approved these changes
@picnixz picnixz Awaiting requested review from picnixz

Uh oh!

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

Assignees

No one assigned

Labels

skip news topic-ctypes

Projects

Docs PRs
Status: Done

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.

5 participants

@junkmd @encukou @hugovk @picnixz @ZeroIntensity
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 01402520 — 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.