Implement basic library logging #231
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced May 16, 2020
peternowee
force-pushed
the
feature/logging
branch
from
a64391e
to
797b86e
Compare
pydot currently prints some error messages to standard output (`stdout`). Printing anything is considered bad practice for libraries [1] [2]. This becomes even more disturbing if the prints contain long Graphviz output or DOT strings. [1]: https://stackoverflow.com/questions/4201856/error-handling-strategies-in-a-shared-library-c [2]: https://www.reddit.com/r/C_Programming/comments/97ebfj/redirect_stdout_and_stderr_to_a_buffer/e47n9kh/ This commit lays the groundwork for switching from printing to logging by providing a basic library logging setup and some user documentation on how to read the logs. Later commits will replace the actual printing by logging. Discussed in pydot#171 and pydot#231.
Notes:
- I am aware that in `__init__.py`, the placement of the logger
registration before the imports of the submodules leads to additional
warnings in code checkers:
From pylint:
C0413: Import "from pydot.exceptions import *" should be placed at the top of the module (wrong-import-position)
C0413: Import "from pydot.core import *" should be placed at the top of the module (wrong-import-position)
From flake8:
E402 module level import not at top of file
E402 module level import not at top of file
I do not really see how we can avoid this if we want to log a first
message early during pydot initialization. Also note that the
concerned imports are all pydot "internal" submodules and that for
`__init__.py` importing those is its main task, so this having a
central place in the file does not seem unnatural.
- In `test/pydot_unittest.py`, added a final `importlib.reload(pydot)`
to prevent subsequent tests from failing, probably caused by one of
the caveats mentioned in:
https://docs.python.org/3/library/importlib.html#importlib.reload
Notes: - Removed the hint that the parent logger can be used to control all pydot loggers, because this is basic knowledge covered by the Python documentation to which we already refer.
peternowee
force-pushed
the
feature/logging
branch
from
797b86e
to
8a327cf
Compare
|
Rebased on master. The first commit is just a minimal rebase of the original. Additional changes are in the remaining commits:
See commits and commit messages for details. All commits titled "Fixup: ...." will be squashed into the first commit ("Implement basic library logging") on merging. Only the last commit will remain separate, because it contains some README cleanup not related to logging. |
|
This pull request has conflicts, please resolve those so that the changes can be evaluated. |
|
All conflicts have been resolved, thanks! |
lkk7
approved these changes
Apr 14, 2024
lkk7
force-pushed
the
feature/logging
branch
2 times, most recently
from
5c75f41
to
db95e2e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
From the commit message:
pydot currently prints some error messages to standard output (
stdout). Printing anything is considered bad practice for libraries (SO, reddit). This becomes even more disturbing if the prints contain long Graphviz output or DOT strings.This commit lays the groundwork for switching from printing to logging by providing a basic library logging setup and some user documentation on how to read the logs. Later commits will replace the actual printing by logging.
Discussion: #171.
Additional remarks:
CC: @ankostis