enthought / enthought/comtypes

Proposal: Fix `NTDebugHandler` and Deprecate `setup_logging` in `logutil.py`.

Open
#920 0 comments 0 reactions 0 assignees View on GitHub
bug tests
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.