Fix getpelican#2938

Get rid of the coerce_overrides() function.
Add the ParseOverrides argparse.Action to parse overrides.
Treat all extra settings values strictly as json values.
Test overrides.
Edit docs and cli help.

RELEASE.md

@@ -0,0 +1,3 @@
+Release type: patch
+
+Fix incorrect parsing of parameters specified via `-e` / `--extra-settings` option flags (#2938).

docs/settings.rst

@@ -9,11 +9,12 @@ line::
 If you used the ``pelican-quickstart`` command, your primary settings file will
 be named ``pelicanconf.py`` by default.
 
-You can also specify extra settings via ``-e`` / ``--extra-settings`` option
-flags, which will override default settings as well as any defined within
-settings files::
+You can also specify settings via ``-e`` / ``--extra-settings`` option
+flags. It will override default settings as well as any defined within the
+setting file. Note that values must follow JSON notation::
+
+    pelican content -e SITENAME='"A site"' READERS='{"html": null}' CACHE_CONTENT=true
 
-    pelican content -e DELETE_OUTPUT_DIRECTORY=true
 
 .. note::
 

pelican/__init__.py

@@ -1,4 +1,5 @@
 import argparse
+import json
 import logging
 import multiprocessing
 import os
@@ -24,7 +25,7 @@
 from pelican.plugins._utils import get_plugin_name, load_plugins
 from pelican.readers import Readers
 from pelican.server import ComplexHTTPRequestHandler, RootedHTTPServer
-from pelican.settings import coerce_overrides, read_settings
+from pelican.settings import read_settings
 from pelican.utils import (FileSystemWatcher, clean_output_dir, maybe_pluralize)
 from pelican.writers import Writer
 
@@ -259,16 +260,29 @@ def __call__(self, parser, namespace, values, option_string):
         parser.exit()
 
 
-class ParseDict(argparse.Action):
+class ParseOverrides(argparse.Action):
     def __call__(self, parser, namespace, values, option_string=None):
-        d = {}
-        if values:
-            for item in values:
-                split_items = item.split("=", 1)
-                key = split_items[0].strip()
-                value = split_items[1].strip()
-                d[key] = value
-        setattr(namespace, self.dest, d)
+        overrides = {}
+        for item in values:
+            try:
+                k, v = item.split("=", 1)
+            except ValueError:
+                raise ValueError(
+                    'Extra settings must be specified as KEY=VALUE pairs '
+                    f'but you specified {item}'
+                )
+            try:
+                overrides[k] = json.loads(v)
+            except json.decoder.JSONDecodeError:
+                raise ValueError(
+                    f'Invalid JSON value: {v}. '
+                    'Values specified via -e / --extra-settings flags '
+                    'must be in JSON notation. '
+                    'Use -e KEY=\'"string"\' to specify a string value; '
+                    '-e KEY=null to specify None; '
+                    '-e KEY=false (or true) to specify False (or True).'
+                )
+        setattr(namespace, self.dest, overrides)
 
 
 def parse_arguments(argv=None):
@@ -366,13 +380,13 @@ def parse_arguments(argv=None):
 
     parser.add_argument('-e', '--extra-settings', dest='overrides',
                         help='Specify one or more SETTING=VALUE pairs to '
-                             'override settings. If VALUE contains spaces, '
-                             'add quotes: SETTING="VALUE". Values other than '
-                             'integers and strings can be specified via JSON '
-                             'notation. (e.g., SETTING=none)',
+                             'override settings. VALUE must be in JSON notation: '
+                             'specify string values as SETTING=\'"some string"\'; '
+                             'booleans as SETTING=true or SETTING=false; '
+                             'None as SETTING=null.',
                         nargs='*',
-                        action=ParseDict
-                        )
+                        action=ParseOverrides,
+                        default={})
 
     args = parser.parse_args(argv)
 
@@ -385,6 +399,8 @@ def parse_arguments(argv=None):
 
 
 def get_config(args):
+    """Builds a config dictionary based on supplied `args`.
+    """
     config = {}
     if args.path:
         config['PATH'] = os.path.abspath(os.path.expanduser(args.path))
@@ -409,7 +425,7 @@ def get_config(args):
     if args.bind is not None:
         config['BIND'] = args.bind
     config['DEBUG'] = args.verbosity == logging.DEBUG
-    config.update(coerce_overrides(args.overrides))
+    config.update(args.overrides)
 
     return config
 

pelican/settings.py

@@ -1,7 +1,6 @@
 import copy
 import importlib.util
 import inspect
-import json
 import locale
 import logging
 import os
@@ -659,25 +658,3 @@ def configure_settings(settings):
             continue            # setting not specified, nothing to do
 
     return settings
-
-
-def coerce_overrides(overrides):
-    if overrides is None:
-        return {}
-    coerced = {}
-    types_to_cast = {int, str, bool}
-    for k, v in overrides.items():
-        if k not in DEFAULT_CONFIG:
-            logger.warning('Override for unknown setting %s, ignoring', k)
-            continue
-        setting_type = type(DEFAULT_CONFIG[k])
-        if setting_type not in types_to_cast:
-            coerced[k] = json.loads(v)
-        else:
-            try:
-                coerced[k] = setting_type(v)
-            except ValueError:
-                logger.debug('ValueError for %s override with %s, try to '
-                             'load as json', k, v)
-                coerced[k] = json.loads(v)
-    return coerced

pelican/tests/test_settings.py

@@ -7,7 +7,7 @@
 
 from pelican.settings import (DEFAULT_CONFIG, DEFAULT_THEME,
                               _printf_s_to_format_field,
-                              coerce_overrides, configure_settings,
+                              configure_settings,
                               handle_deprecated_settings, read_settings)
 from pelican.tests.support import unittest
 
@@ -304,18 +304,3 @@ def test_deprecated_slug_substitutions_from_file(self):
                          [(r'C\+\+', 'cpp')] +
                          self.settings['SLUG_REGEX_SUBSTITUTIONS'])
         self.assertNotIn('SLUG_SUBSTITUTIONS', settings)
-
-    def test_coerce_overrides(self):
-        overrides = coerce_overrides({
-            'ARTICLE_EXCLUDES': '["testexcl"]',
-            'READERS': '{"foo": "bar"}',
-            'STATIC_EXCLUDE_SOURCES': 'true',
-            'THEME_STATIC_DIR': 'theme',
-            })
-        expected = {
-            'ARTICLE_EXCLUDES': ["testexcl"],
-            'READERS': {"foo": "bar"},
-            'STATIC_EXCLUDE_SOURCES': True,
-            'THEME_STATIC_DIR': 'theme',
-        }
-        self.assertDictEqual(overrides, expected)