-
Notifications
You must be signed in to change notification settings - Fork 832
Refactoring the logging module #5367
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: develop
Are you sure you want to change the base?
Changes from 11 commits
33a8005
1bf0ee1
24a6c31
7a5c0b3
f1be0df
dbcfb1c
93966fb
c5f6133
e30d0ab
5a5906d
f679251
2ba1c2e
cca9258
94f056f
7f5812f
ba9315e
0e36231
e63f5ef
a81e4ad
f25aec7
bdaf241
ab00172
d0b732f
ec0ed25
6d1a675
74bcc03
0fdd02e
d952902
c34eca5
f9a4961
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,22 +86,30 @@ | |
| """ | ||
| import sys | ||
| import logging | ||
| import re | ||
| import os | ||
|
|
||
| from tqdm.auto import tqdm | ||
|
|
||
| from .. import version | ||
|
|
||
|
|
||
| def start_logging(logfile="MDAnalysis.log", version=version.__version__): | ||
| def start_logging(stream="MDAnalysis.log", version=version.__version__): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comment above in line 101. Why would a user want to manually change the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fair. (I think this was a pattern where we wanted to avoid accessing global scope inside functions and passed the values through kwargs.) |
||
| """Start logging of messages to file and console. | ||
|
|
||
| The default logfile is named `MDAnalysis.log` and messages are | ||
| logged with the tag *MDAnalysis*. | ||
| """ | ||
| create("MDAnalysis", logfile=logfile) | ||
| logging.getLogger("MDAnalysis").info( | ||
|
jauy123 marked this conversation as resolved.
|
||
| "MDAnalysis %s STARTED logging to %r", version, logfile | ||
| create( | ||
| "MDAnalysis", | ||
| stream=stream, | ||
| level="DEBUG", | ||
| fmt="%(asctime)s %(name)-12s %(levelname)-8s %(message)s", | ||
| ) | ||
| create( | ||
| "MDAnalysis", | ||
| stream=sys.stdout, | ||
| level="INFO", | ||
| fmt="%(name)-12s: %(levelname)-8s %(message)s", | ||
| ) | ||
|
Comment on lines
+110
to
+121
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of this is intend to replicate old behavior from the original. |
||
|
|
||
|
|
||
|
|
@@ -112,7 +120,9 @@ def stop_logging(): | |
| clear_handlers(logger) # this _should_ do the job... | ||
|
|
||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general, I guess this PR make
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably yes to renaming, but you'd need to
because we're following semantic versioning https://semver.org and we cannot remove documented functionality in a public function until we make a new major release. Similarly, we cannot just rename kwargs in a public function like create(). You have to keep the old one around and deprecated them.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably should raise an issue of it as well |
||
| def create(logger_name="MDAnalysis", logfile="MDAnalysis.log"): | ||
| def create( | ||
| logger_name="MDAnalysis", stream="MDAnalysis.log", level="DEBUG", fmt=None | ||
|
jauy123 marked this conversation as resolved.
Outdated
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as above, you need to deprecate kwargs if you really want to rename them |
||
| ): | ||
| """Create a top level logger. | ||
|
|
||
| - The file logger logs everything (including DEBUG). | ||
|
|
@@ -132,23 +142,27 @@ def create(logger_name="MDAnalysis", logfile="MDAnalysis.log"): | |
|
|
||
| logger = logging.getLogger(logger_name) | ||
|
|
||
| logger.setLevel(logging.DEBUG) | ||
| logger.setLevel(level.upper()) | ||
|
|
||
| # handler that writes to logfile | ||
| logfile_handler = logging.FileHandler(logfile) | ||
| logfile_formatter = logging.Formatter( | ||
| "%(asctime)s %(name)-12s %(levelname)-8s %(message)s" | ||
| ) | ||
| logfile_handler.setFormatter(logfile_formatter) | ||
| logger.addHandler(logfile_handler) | ||
|
|
||
| # define a Handler which writes INFO messages or higher to the sys.stderr | ||
| console_handler = logging.StreamHandler() | ||
| console_handler.setLevel(logging.INFO) | ||
| # set a format which is simpler for console use | ||
| formatter = logging.Formatter("%(name)-12s: %(levelname)-8s %(message)s") | ||
| console_handler.setFormatter(formatter) | ||
| logger.addHandler(console_handler) | ||
| # https://docs.python.org/3/library/logging.handlers.html#streamhandler | ||
| # | ||
| # The StreamHandler class, located in the core logging package, | ||
|
jauy123 marked this conversation as resolved.
Outdated
|
||
| # sends logging output to streams such as sys.stdout, sys.stderr or | ||
| # any file-like object (or, more precisely, any object which supports | ||
| # write() and flush() methods). | ||
|
|
||
| # This only check the existance and not the functionality. Should be ok? | ||
| if hasattr(stream, "write") and hasattr(stream, "flush"): | ||
| handler = logging.StreamHandler(stream) | ||
| elif isinstance(stream, (str, os.PathLike)): | ||
| handler = logging.FileHandler(stream) | ||
| else: | ||
| raise TypeError("Input Stream is neither a file or a stream") | ||
|
|
||
| handler.setFormatter(logging.Formatter(fmt)) | ||
| logger.addHandler(handler) | ||
|
|
||
| logger.info(f"MDAnalysis {version} STARTED logging to {stream!r}") | ||
|
jauy123 marked this conversation as resolved.
Outdated
|
||
|
|
||
| return logger | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under semantic versioning, you need to maintain backwards compatibility. So something like