Clean up main and improve error handling

PFERD/__main__.py

@@ -4,15 +4,13 @@ import configparser
 from pathlib import Path
 
 from .cli import PARSER, load_default_section
-from .config import Config, ConfigDumpException, ConfigLoadException
+from .config import Config, ConfigDumpError, ConfigLoadError, ConfigOptionError
 from .logging import log
 from .pferd import Pferd
 from .version import NAME, VERSION
 
 
-def load_parser(
+def load_config_parser(args: argparse.Namespace) -> configparser.ConfigParser:
-        args: argparse.Namespace,
-) -> configparser.ConfigParser:
     log.explain_topic("Loading config")
     parser = configparser.ConfigParser()
 
@@ -47,46 +45,88 @@ def prune_crawlers(
     # TODO Check if crawlers actually exist
 
 
-def main() -> None:
+def load_config(args: argparse.Namespace) -> Config:
-    args = PARSER.parse_args()
+    try:
+        return Config(load_config_parser(args))
+    except ConfigLoadError as e:
+        log.error(str(e))
+        log.error_contd(e.reason)
+        exit(1)
 
-    # Configure log levels set by command line arguments
+
+def configure_logging_from_args(args: argparse.Namespace) -> None:
     if args.explain is not None:
         log.output_explain = args.explain
-    if args.dump_config:
+
+    # We want to prevent any unnecessary output if we're printing the config to
+    # stdout, otherwise it would not be a valid config file.
+    if args.dump_config == "-":
         log.output_explain = False
 
+
+def configure_logging_from_config(args: argparse.Namespace, config: Config) -> None:
+    # In configure_logging_from_args(), all normal logging is already disabled
+    # whenever we dump the config. We don't want to override that decision with
+    # values from the config file.
+    if args.dump_config == "-":
+        return
+
+    try:
+        if args.explain is None:
+            log.output_explain = config.default_section.explain()
+    except ConfigOptionError as e:
+        log.error(str(e))
+        exit(1)
+
+
+def dump_config(args: argparse.Namespace, config: Config) -> None:
+    try:
+        if args.dump_config is True:
+            config.dump()
+        elif args.dump_config == "-":
+            config.dump_to_stdout()
+        else:
+            config.dump(Path(args.dump_config))
+    except ConfigDumpError as e:
+        log.error(str(e))
+        log.error_contd(e.reason)
+        exit(1)
+
+
+def main() -> None:
+    args = PARSER.parse_args()
+
     if args.version:
         print(f"{NAME} {VERSION}")
         exit()
 
-    try:
+    # Configuring logging happens in two stages because CLI args have
-        config = Config(load_parser(args))
+    # precedence over config file options and loading the config already
-    except ConfigLoadException as e:
+    # produces some kinds of log messages (usually only explain()-s).
-        log.error(f"Failed to load config file at path {str(e.path)!r}")
+    configure_logging_from_args(args)
-        log.error_contd(f"Reason: {e.reason}")
-        exit(1)
 
-    # Configure log levels set in the config file
+    config = load_config(args)
-    # TODO Catch config section exceptions
+
-    if args.explain is None:
+    # Now, after loading the config file, we can apply its logging settings in
-        log.output_explain = config.default_section.explain()
+    # all places that were not already covered by CLI args.
+    configure_logging_from_config(args, config)
 
     if args.dump_config is not None:
-        try:
+        dump_config(args, config)
-            if args.dump_config is True:
-                config.dump()
-            elif args.dump_config == "-":
-                config.dump_to_stdout()
-            else:
-                config.dump(Path(args.dump_config))
-        except ConfigDumpException:
-            exit(1)
         exit()
 
+    # TODO Unset exclusive output on exceptions (if it was being held)
     pferd = Pferd(config)
     try:
         asyncio.run(pferd.run())
     except KeyboardInterrupt:
+        log.explain_topic("Interrupted, exiting immediately")
+        log.explain("Open files and connections are left for the OS to clean up")
+        log.explain("Temporary files are not cleaned up")
         # TODO Clean up tmp files
-        pass
+        # And when those files *do* actually get cleaned up properly,
+        # reconsider what exit code to use here.
+        exit(1)
+    except Exception:
+        log.unexpected_exception()
+        exit(1)

PFERD/config.py

@@ -2,7 +2,6 @@ import asyncio
 import os
 import sys
 from configparser import ConfigParser, SectionProxy
-from dataclasses import dataclass
 from pathlib import Path
 from typing import Any, List, NoReturn, Optional, Tuple
 
@@ -10,21 +9,34 @@ from .logging import log
 from .utils import prompt_yes_no
 
 
-@dataclass
+class ConfigLoadError(Exception):
-class ConfigLoadException(Exception):
+    """
-    path: Path
+    Something went wrong while loading the config from a file.
-    reason: str
+    """
+
+    def __init__(self, path: Path, reason: str):
+        super().__init__(f"Failed to load config from {path}")
+        self.path = path
+        self.reason = reason
 
 
-class ConfigDumpException(Exception):
+class ConfigOptionError(Exception):
-    pass
+    """
+    An option in the config file has an invalid or missing value.
+    """
+
+    def __init__(self, section: str, key: str, desc: str):
+        super().__init__(f"Section {section!r}, key {key!r}: {desc}")
+        self.section = section
+        self.key = key
+        self.desc = desc
 
 
-@dataclass
+class ConfigDumpError(Exception):
-class ConfigFormatException(Exception):
+    def __init__(self, path: Path, reason: str):
-    section: str
+        super().__init__(f"Failed to dump config to {path}")
-    key: str
+        self.path = path
-    desc: str
+        self.reason = reason
 
 
 class Section:
@@ -36,7 +48,7 @@ class Section:
         self.s = section
 
     def error(self, key: str, desc: str) -> NoReturn:
-        raise ConfigFormatException(self.s.name, key, desc)
+        raise ConfigOptionError(self.s.name, key, desc)
 
     def invalid_value(
             self,
@@ -83,7 +95,7 @@ class Config:
     @staticmethod
     def load_parser(parser: ConfigParser, path: Optional[Path] = None) -> None:
         """
-        May throw a ConfigLoadException.
+        May throw a ConfigLoadError.
         """
 
         if path:
@@ -99,21 +111,15 @@ class Config:
             with open(path) as f:
                 parser.read_file(f, source=str(path))
         except FileNotFoundError:
-            raise ConfigLoadException(path, "File does not exist")
+            raise ConfigLoadError(path, "File does not exist")
         except IsADirectoryError:
-            raise ConfigLoadException(path, "That's a directory, not a file")
+            raise ConfigLoadError(path, "That's a directory, not a file")
         except PermissionError:
-            raise ConfigLoadException(path, "Insufficient permissions")
+            raise ConfigLoadError(path, "Insufficient permissions")
-
-    @staticmethod
-    def _fail_dump(path: Path, reason: str) -> None:
-        print(f"Failed to dump config file to {path}")
-        print(f"Reason: {reason}")
-        raise ConfigDumpException()
 
     def dump(self, path: Optional[Path] = None) -> None:
         """
-        May throw a ConfigDumpException.
+        May throw a ConfigDumpError.
         """
 
         if not path:
@@ -124,7 +130,7 @@ class Config:
         try:
             path.parent.mkdir(parents=True, exist_ok=True)
         except PermissionError:
-            self._fail_dump(path, "Could not create parent directory")
+            raise ConfigDumpError(path, "Could not create parent directory")
 
         try:
             # Ensuring we don't accidentally overwrite any existing files by
@@ -140,11 +146,11 @@ class Config:
                     with open(path, "w") as f:
                         self._parser.write(f)
                 else:
-                    self._fail_dump(path, "File already exists")
+                    raise ConfigDumpError(path, "File already exists")
         except IsADirectoryError:
-            self._fail_dump(path, "That's a directory, not a file")
+            raise ConfigDumpError(path, "That's a directory, not a file")
         except PermissionError:
-            self._fail_dump(path, "Insufficient permissions")
+            raise ConfigDumpError(path, "Insufficient permissions")
 
     def dump_to_stdout(self) -> None:
         self._parser.write(sys.stdout)

PFERD/logging.py

@@ -1,4 +1,6 @@
 import asyncio
+import sys
+import traceback
 from contextlib import asynccontextmanager, contextmanager
 # TODO In Python 3.9 and above, ContextManager and AsyncContextManager are deprecated
 from typing import AsyncIterator, ContextManager, Iterator, List, Optional
@@ -110,6 +112,27 @@ class Log:
     def error_contd(self, text: str) -> None:
         self.print(f"[red]{escape(text)}")
 
+    def unexpected_exception(self) -> None:
+        t, v, tb = sys.exc_info()
+
+        self.error("An unexpected exception occurred")
+        self.error_contd("")
+
+        for line in traceback.format_tb(tb):
+            self.error_contd(line[:-1])  # Without trailing newline
+
+        if str(v):
+            self.error_contd(f"{t.__name__}: {v}")
+        else:
+            self.error_contd(t.__name__)
+
+        self.error_contd("")
+        self.error_contd("""
+An unexpected exception occurred. This usually shouldn't happen. Please copy
+your program output and send it to the PFERD maintainers, either directly or as
+a GitHub issue: https://github.com/Garmelon/PFERD/issues/new
+        """.strip())
+
     def explain_topic(self, text: str) -> None:
         if self.output_explain:
             self.print(f"[cyan]{escape(text)}")