Log debug message if import PythonShell and PythonEditor fail in api.py #801
Conversation
| import logging | ||
|
|
||
|
|
||
| _PYFACE_LOGGER = logging.getLogger("pyface") |
There was a problem hiding this comment.
This makes me a bit twitchy, having the logger being used hard-coded. Perhaps better would be to pass in the logger instance to optional_import so it can use the logger of the module where optional_import is called from.
There was a problem hiding this comment.
It would also make testing easier
pyface/api.py
Outdated
| from .util._optional_dependencies import optional_import as _optional_import | ||
|
|
||
| # Excuse pygments dependency (for Qt), otherwise re-raise | ||
| with _optional_import("pygments", msg="PythonEditor not available"): |
There was a problem hiding this comment.
The log message doesn't mention that the reason it isn't available is because Pygments isn't installed - that would be useful to know.
| yield | ||
| except ImportError as exception: | ||
| if exception.name == dependency_name: | ||
| _PYFACE_LOGGER.debug(msg, exc_info=True) |
There was a problem hiding this comment.
Maybe info rather than debug?
There was a problem hiding this comment.
I still prefer debug because this is intended for debugging purposes. But that is not a strong preference.
There was a problem hiding this comment.
I think typically info message does not contain exception traceback info (the exc_info=True) bit. Here I think we want to show the traceback which will include the ImportError on pygments.
I could do both, I suppose?
Like this?
Lines 240 to 241 in 7e32dbf
Codecov Report
@@ Coverage Diff @@
## master #801 +/- ##
==========================================
+ Coverage 40.67% 40.91% +0.24%
==========================================
Files 508 509 +1
Lines 27832 27846 +14
Branches 4217 4218 +1
==========================================
+ Hits 11320 11393 +73
+ Misses 16016 15979 -37
+ Partials 496 474 -22
Continue to review full report at Codecov.
|
|
Thank you @corranwebster |
Follow up from #796
In particular, this: #796 (comment)
If PythonEditor and PythonShell cannot be imported because pygments is not available, it might help if we log a debug message so that downstream projects have more information when the import fails.
The alternative would be to emit a warning, but for developers who don't care about these two classes, this could be too noisy.