-
-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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.
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
gh-126615: Make COMError
public and add to ctypes
doc.
#126686
Conversation
58cc032
to
90fb47f
Compare
Doc/library/ctypes.rst
Outdated
|
||
.. exception:: COMError(hresult, text, details) | ||
|
||
Windows only: This non-public exception is raised when a COM method call |
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)
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?
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.
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.
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"?
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 :)
Doc/library/ctypes.rst
Outdated
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) |
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?
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.
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.
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:
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.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
in order to maintain consistency with other sections.
AFAICS, |
Doc/library/ctypes.rst
Outdated
|
||
.. attribute:: details | ||
|
||
The 5-tuple representing additional details about the error. |
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?
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 IErrorInfo
1: 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
-
Reference:
IErrorInfo
implementation incomtypes
. ↩
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.
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.
Doc/library/ctypes.rst
Outdated
@@ -1799,10 +1793,14 @@ 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 |
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
.
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 |
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.
I DO agree that 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. |
I see where you're coming from, but I don't think this PR should be merged as-is. It's true that While But for 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 |
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 Since this is no longer an internal change, I’ll also add a NEWS entry. Additionally, I’ll include a |
COMError
to ctypes
doc.COMError
public and add to ctypes
doc.
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:
Doc/library/ctypes.rst
Outdated
.. versionchanged:: 3.14 | ||
|
||
This exception is now public. | ||
Previously, this was private and only available in ``_ctypes``. |
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.
.. versionchanged:: 3.14 | |
This exception is now public. | |
Previously, this was private and only available in ``_ctypes``. | |
.. versionadded:: next |
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.
Doc/library/ctypes.rst
Outdated
@@ -1799,10 +1793,14 @@ 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, an :exc:`OSError` is raised if the COM method | |||
call fails. If *iid* is specified, a :exc:`.COMError` is raised instead. |
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:
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. |
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!
Please refer also to gh-126384 and gh-126610 for the behavior when a foreign function fails to call a COM method.
ctypes
: MakeCOMError
, mentioned in the "Function prototypes" section of documentation, public. #126615📚 Documentation preview 📚: https://cpython-previews--126686.org.readthedocs.build/