-
Notifications
You must be signed in to change notification settings - Fork 71
Add typing and fix some bugs I found on the way #166
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
base: main
Are you sure you want to change the base?
Conversation
…2 to detect errors
Also applied upgrade_pythoncapi.py to source.
| for name, value in defs: | ||
| print(f' .. autoattribute:: systemd.id128.{name}', file=out) | ||
|
|
||
| old = open(stubfile).read().splitlines() |
Check warning
Code scanning / CodeQL
File is not always closed Warning
| flags: OpenFlag | None = None, | ||
| *, | ||
| files: _typing.Sequence[_typeshed.FileDescriptorOrPath], | ||
| converters: dict[str, _typing.Callable[[_typing.Any], _typing.Any]] | None = None) -> None: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
| flags: OpenFlag | None = None, | ||
| *, | ||
| namespace: _typeshed.StrOrBytesPath, | ||
| converters: dict[str, _typing.Callable[[_typing.Any], _typing.Any]] | None = None) -> None: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
| path: _typeshed.FileDescriptorOrPath | None = None, | ||
| files: None = None, | ||
| converters: dict[str, _typing.Callable[[_typing.Any], _typing.Any]] | None = None, | ||
| namespace: None = None) -> None: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
|
|
||
| if _typing.TYPE_CHECKING: | ||
| class SenderFunction(_typing.Protocol): | ||
| def __call__(self, MESSAGE: str, **kwargs: _typing.Any) -> None: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
f0e9f6e to
2ba4c4f
Compare
…e to prevent leaked references PyModule_AddObject does not release a reference if it fails.
This is what CPython's built-in modules do and what the reference says: https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_name.
…f Reader constructor
The constructor finishes successfully with an empty argument, but it causes a SystemError, because the call to PySequence_GetItem() failed.
The constructor used to raise a MemoryError if files[0] returned an int, but files.__length__() did not exist.
sd_id128_from_string() does not treat NULL specially, so there is no reason to allow it.
All constants are added to the module to prevent breaking current users.
All code changes should not impact current users. Force Sphinx to use the docstrings from the actual extension modules, otherwise it would only parse the new stub files.
|
I've only had a cursory first look at this PR. Thanks for all the work! Is there a reason you put the type annoations in a
That's a pretty big thing and I'm not entirely sure it's worth it to support 3.9. What version did you incorporate, would it maybe be better to do it via a submodule, or is there a way to whittle it down, e.g. only use the definitions actually necessary? |
There is no way (as far as I know) to put inline type annotations in a compiled extension module. In the .py files I did add the annotations inline.
|
I wanted to add typing to the Python and extension modules to make the library easier to use in IDEs, but I encountered some bugs on the way there.
The only actual functional changes to the Python and C code are:
path,files, andnamespacearguments to the constructor ofsystemd._reader.Reader.Noneas an argument tosystemd._reader.get_catalog. This already produced an error, becausesd_id128_from_string()already fails with aNULLargument, but now the error comes from the Python argument parsing instead.systemd._reader.Readerand the return values ofsystemd._reader.Reader.wait()andsystemd._reader.Reader.process()into Python enums. Because they areIntFlagandIntEnumrespectively, they are still subtypes ofint, so users should not be affected.systemd.daemon._convert_fileobj(). It now checks if the argument is anint, callingarg.fileno()otherwise, instead of always tryingarg.fileno()first.systemd.journalto only use.hexif a value is auuid.UUIDinstead of checking for the existence of ahexattribute.bytesdoes have.hex(), a method, which means the underlying function would get called with a bound function instead of thebytesobject.systemd.daemon'sis_socket_...functions'typeargument toNone, so they can be typed assocket.AddressFamily | None. Passing 0 at runtime still works the same as before, because the typing is not used at runtime.The change from
PyModule_AddObjecttoPyModule_Add,PyModule_AddObjectRef, andPyModule_AddTypemakes it easier to fix leaked references. In chains ofPyModule_AddObjectif adding one object fails, the reference count of that object has to be decremented manually afterwards (https://docs.python.org/3/c-api/module.html#c.PyModule_AddObject). UsingPyModule_Addalways decrements the reference count andPyModule_AddObjectRefallows using the cleanup attribute to automatically decrement the original reference count at the end of the block, regardless of the success of adding the object to the module. BecausePyModule_Addis Python 3.13+ andPyModule_AddObjectRefis Python 3.10+, I added the pythoncapi-compat header to still be able to support older versions.