Skip to content

gh-126615: Make COMError public and add to ctypes doc. #126686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Nov 20, 2024
Merged

gh-126615: Make COMError public and add to ctypes doc. #126686

Changes from 5 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
e70aa54
add `COMError` exception to `Doc/library/ctypes.rst`.
junkmd Nov 16, 2024
181f512
Add a mention of the difference in exceptions that can be raised.
junkmd Nov 16, 2024
857fef5
Add a dot to refer to the object within the same module.
junkmd Nov 16, 2024
90fb47f
Simplify descriptions of attributes.
junkmd Nov 16, 2024
1f9d4ee
Align with the docstring defined in the C implementation:
junkmd Nov 16, 2024
b71e310
Apply suggestions from code review
junkmd Nov 16, 2024
dafb5b2
Add the dot to make the reflink.
junkmd Nov 16, 2024
49d5e5f
Add the `availability`.
junkmd Nov 16, 2024
1fa159a
Create the "Exceptions" section.
junkmd Nov 16, 2024
934c36a
Remove the `availability` directive
junkmd Nov 16, 2024
9461642
`WindowsError` -> `OSError`
junkmd Nov 18, 2024
705ffcc
details of `details`.
junkmd Nov 18, 2024
b5a2170
Small fix.
junkmd Nov 18, 2024
a16f1e2
Make `COMError` public.
junkmd Nov 19, 2024
e2617f3
Update docs.
junkmd Nov 19, 2024
57d76c5
Update `test_COMError`.
junkmd Nov 19, 2024
36f1607
Update the NEWS.
junkmd Nov 20, 2024
068ea2a
`.. versionchanged::` -> `.. versionadded::`
junkmd Nov 20, 2024
3da9e15
Update the `prototype` section.
junkmd Nov 20, 2024
920bccc
Merge branch 'main' into gh_126615_comerror_doc
junkmd Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions Doc/library/ctypes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1799,10 +1799,31 @@ different ways, depending on the type and number of the parameters in the call:
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
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

Thank you.

call fails. If *iid* is specified, a :exc:`.COMError` is raised instead.
Copy link
Member

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.


COM methods use a special calling convention: They require a pointer to
the COM interface as first argument, in addition to those parameters that
are specified in the :attr:`!argtypes` tuple.


.. exception:: COMError(hresult, text, details)
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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

Copy link
Contributor Author

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.


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

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)

Copy link
Contributor Author

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?

Copy link
Member

@picnixz picnixz Nov 16, 2024 •
edited
Loading

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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"?

Copy link
Member

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 :)

failed.

.. attribute:: hresult

The integer value representing the error code.

.. attribute:: text

The error message.

.. attribute:: details

The 5-tuple representing additional details about the error.

The optional *paramflags* parameter creates foreign function wrappers with much
more functionality than the features described above.

Expand Down
Loading