Refactoring the logging module

package/MDAnalysis/__init__.py

@@ -200,7 +200,7 @@
 from .lib import log
 from .lib.log import start_logging, stop_logging
 
-logging.getLogger("MDAnalysis").addHandler(log.NullHandler())
+logging.getLogger("MDAnalysis").addHandler(logging.NullHandler())
 del logging
 
 # only MDAnalysis DeprecationWarnings are loud by default

package/MDAnalysis/lib/log.py

@@ -86,22 +86,41 @@
 """
 import sys
 import logging
-import re
+import os
 
 from tqdm.auto import tqdm
 
+# from MDAnalysis.lib.util import deprecate
+
 from .. import version
 
+# Things to deprecated:
+# logfile -> stream (bc it could be any stream like object)
+#
+# version? Why would any user want to modify version in the first place?
+# Just have the message logged it directly
+
 
-def start_logging(logfile="MDAnalysis.log", version=version.__version__):
+def start_logging(stream="MDAnalysis.log", version=version.__version__):
     """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(
-        "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",
+    )
+    logging.getLogger(__name__).info(
+        f"MDAnalysis {version} STARTED logging to {stream!r}"
     )
 
 
@@ -112,7 +131,30 @@ def stop_logging():
     clear_handlers(logger)  # this _should_ do the job...
 
 
-def create(logger_name="MDAnalysis", logfile="MDAnalysis.log"):
+# Things to deprecated overall:
+# log.NullHandler -> replace with logging.NullHandler() warning
+# create() -> add_handler() (potentially confusing bc standard library
+#                            has the same method but in camelcase weirdly?)
+#                            or create_handler()?
+#
+# For create():
+# logfile -> stream (bc it could be any stream like object, not just logfiles)
+# logger_name should be deprecated.
+#
+# Standard library recommends constructing through
+# logging.getLogger(__name__) because all loggers share the same namespace
+# and this is a systemmatic way of defining loggers
+#
+# __name__ is MDAnalysis
+# See 2nd paragraph: https://docs.python.org/3/library/logging.html#logger-objects
+#
+def create(
+    logger_name="MDAnalysis",
+    stream="MDAnalysis.log",
+    level="DEBUG",
+    fmt=None,
+    mode="a",
+):
     """Create a top level logger.
 
     - The file logger logs everything (including DEBUG).
@@ -130,25 +172,23 @@ def create(logger_name="MDAnalysis", logfile="MDAnalysis.log"):
        http://docs.python.org/library/logging.html?#logging-to-multiple-destinations
     """
 
+    # replaced with logging.getLogger(__name__)
     logger = logging.getLogger(logger_name)
 
-    logger.setLevel(logging.DEBUG)
-
-    # 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)
+    # This checks for file-like object per duck typing
+    # https://docs.python.org/3/library/logging.handlers.html#streamhandler
+    if hasattr(stream, "write") and hasattr(stream, "flush"):
+        handler = logging.StreamHandler(stream)
+    elif isinstance(stream, (str, os.PathLike)):
+        handler = logging.FileHandler(stream, mode=mode)
+    else:
+        raise TypeError(
+            "Input Stream is neither a string, PathLike object or a stream"
+        )
+
+    handler.setLevel(level.upper())
+    handler.setFormatter(logging.Formatter(fmt))
+    logger.addHandler(handler)
 
     return logger
 
@@ -159,27 +199,10 @@ def clear_handlers(logger):
     (only important for reload/debug cycles...)
 
     """
-    for h in logger.handlers:
+    for h in list(logger.handlers):
         logger.removeHandler(h)
 
 
-class NullHandler(logging.Handler):
-    """Silent Handler.
-
-    Useful as a default::
-
-      h = NullHandler()
-      logging.getLogger("MDAnalysis").addHandler(h)
-      del h
-
-    see the advice on logging and libraries in
-    http://docs.python.org/library/logging.html?#configuring-logging-for-a-library
-    """
-
-    def emit(self, record):
-        pass
-
-
 class ProgressBar(tqdm):
     r"""Display a visual progress bar and time estimate.
 

testsuite/MDAnalysisTests/lib/test_log.py

@@ -20,12 +20,88 @@
 # MDAnalysis: A Toolkit for the Analysis of Molecular Dynamics Simulations.
 # J. Comput. Chem. 32 (2011), 2319--2327, doi:10.1002/jcc.21787
 #
-import warnings
+
 import pytest
+import sys
+import logging
+import MDAnalysis as mda
 
+from os.path import basename
 from MDAnalysis.lib.log import ProgressBar
 
 
+class TestConvenienceFunctions:
+    def test_start_logging(self, tmp_path):
+        mda.start_logging(tmp_path / "MDAnalysis.log")
+        logger = logging.getLogger("MDAnalysis")
+
+        # Test expected handlers' presence and behavior
+        assert any(isinstance(h, logging.NullHandler) for h in logger.handlers)
+        any(
+            isinstance(h, logging.FileHandler)
+            and basename(h.stream.name) == "MDAnalysis.log"
+            and h.level == logging.DEBUG
+            for h in logger.handlers
+        )
+        assert any(
+            isinstance(h, logging.StreamHandler)
+            and h.stream is sys.stdout
+            and h.level == logging.INFO
+            for h in logger.handlers
+        )
+
+    def test_stop_logging(self, tmp_path):
+        mda.lib.log.start_logging(tmp_path / "MDAnalysis.log")
+        logger = logging.getLogger("MDAnalysis")
+        mda.lib.log.stop_logging()
+
+        assert len(logger.handlers) == 0
+
+def test_message_console(tmp_path):
+    pass
+
+def test_message_file(tmp_path):
+    pass
+
+# TODO need to make a fixture that can clear all handlers per test
+class TestCreateBehaviors:
+
+    def test_input_path(self, tmp_path):
+        mda.lib.log.create(stream=tmp_path / "foo.log")
+
+        assert (tmp_path / "foo.log").exists()
+
+    def test_input_string(self, tmp_path):
+        mda.lib.log.create(stream=str(tmp_path / "foo.log"))
+
+        assert (tmp_path / "foo.log").exists()
+
+    # NOTE Assert state could be cleaned up after clear_handlers() fixture is implemented
+    def test_input_stream(self):
+        mda.lib.log.create(stream=sys.stdout)
+
+        logger = logging.getLogger("MDAnalysis")
+        assert any(
+            isinstance(h, logging.StreamHandler) and h.stream is sys.stdout
+            for h in logger.handlers
+        )
+
+    def test_exception(tmp_path):
+        with pytest.raises(
+            TypeError,
+            match="Input Stream is neither a string, PathLike object or a stream",
+        ):
+            mda.lib.log.create(stream=2)
+
+def test_level_parameter():
+    pass
+
+def test_fmt_parameter():
+    pass
+
+def test_mode_parameter():
+    pass
+
 class TestProgressBar(object):
 
     def test_output(self, capsys):