enthought / enthought/comtypes
Proposal: Fix `NTDebugHandler` and Deprecate `setup_logging` in `logutil.py`.
- Dominant language
- Python
- Stars
- 345
- Forks
- 105
- PR merge metrics
- No merged PRs in 30d
Description
## Summary
The `logutil` module contains components that are either broken, or overly broad in this project responsibilities, and have not been properly maintained or tested for a long time.
I propose to fix the `NTDebugHandler` class and deprecate the `setup_logging` function.
## Problems
### 1. `NTDebugHandler` is broken due to Python 2/3 `str` vs `bytes` incompatibility
https://github.com/enthought/comtypes/blob/06879ab77bd71d86ca2601ce0af01c3e3f85de26/comtypes/logutil.py#L17-L28
* In Python 2, `str` represented byte sequences, and Unicode strings were handled by a separate `unicode` type.
In Python 3, `str` represents Unicode text, and `bytes` is used explicitly for byte sequences.
* (Reference: https://docs.python.org/3/whatsnew/3.0.html#text-vs-data-instead-of-unicode-vs-8-bit)
* In Python 3, `logging.Formatter.format`, which is invoked by `logging.Logger.format`, always returns a Unicode string (`str`).
* The `NTDebugHandler.emit` method has a type check (`isinstance(text, str)`) and passing this `str` directly to `_OutputDebugStringA` (which expects `LPCSTR`(`bytes`)) inevitably leads to errors.
* So `_OutputDebugStringW` (for wide character strings) is sufficient. The existing type branching is now a source of malfunction.
### 2. `setup_logging` has excessive responsibility
* The `setup_logging` is a just `logging` utility, unrelated to COM or Windows.
* Such `logging` configuration responsibilities should ideally rest with the application developer.
### 3. Lack of usage and redundancy
* These components are likely unused or used in very limited circumstances for a long time.
* `NTDebugHandler` is used in `server/inprocserver.py`, but as discussed in https://github.com/enthought/comtypes/issues/671, the use case for `inprocserver` has not been confirmed for years.
This suggests either that its breakage has gone unnoticed, or developers are using forks/custom patches.
* The `setup_logging` function is not called anywhere within `server/inprocserver.py`, indicating its non-essential nature for the project and a weak justification for its existence.
## Proposal
For the reasons outlined above, we propose to fix `NTDebugHandler` and deprecate the `setup_logging` function from this library.
This will simplify the codebase, resolve potential Python version incompatibility issues, and clarify the scope of this project responsibilities.
I plan to fix `NTDebugHandler` by the next release and deprecate `setup_logging`, with a view to removing it in a future version (likely 1.5.0).
Contributor guide
Assessment
This issue has not been assessed yet.