Repository: hukkin/mdformat

Issue:
Warning on CLI argument default
### Describe the bug

**context**

When `mdformat` or a plugin add a cli argument with a default (e.g. 'action=store_false', 'action=store_true', `def=True`), the stored default will override any settings read from the TOML file

Previously caused me issues from https://github.com/KyleKing/mdformat-mkdocs/commit/5c7be8c7fea5c6f3199d4a203a443bd724f7e1be and https://github.com/hukkin/mdformat/pull/496#discussion_r1891346960.

**bug**

When using `store_false` and running `mdformat .`, the plugin option appears as False even if found in `.mdformat.toml` because of how the dictionaries are merged

**fix**

Replacing `store_false` with `store_const` and `const=False` will remove the default and behave as expected (when no CLI argument is passed, the TOML option takes precedence.

### Reproduce the bug

1. Use `store_true`
1. Create `.mdformat.toml` with configuration option set to True
1. Run `mdformat tmp.md`
1. Configuration option was resolved as False

### List your environment

```
mdformat 0.7.21 (mdformat_frontmatter 2.0.8, mdformat-gfm 0.4.1, mdformat_simple_breaks 0.0.1, mdformat_tables 1.0.0, mdformat_mkdocs 4.1.2, mdformat_footnote 0.1.1, mdformat_wikilink 0.2.0, mdformat-config 0.2.1, mdformat-web 0.2.0, mdformat-beautysh 0.1.1, mdformat-ruff 0.1.3)   
```

### Proposal

Could `mdformat` output a warning when any CLI argument is configured with a default to prevent this footgun? The remediation is straightforward, but the root cause is non-obvious for plugin developers

As a quick proof of concept, something like:

```py
import argparse

parser = argparse.ArgumentParser()
group = parser.add_argument_group(title="group")

# Only `--correct` will work with `.mdformat.toml`
parser.add_argument("--yes", action="store_true")
group.add_argument("--no", action="store_false")
parser.add_argument("--correct", action="store_const", const=True)

# Could check all arguments and not just plugins, but I'm not sure if there is a non-private way to access them
for action in parser._actions:
    # Would want a better way to differentiate _HelpAction from others
    if action.default not in {None, "==SUPPRESS=="}:
        print("Found action with default", action)

print("Parsed:", parser.parse_args())
```

Discussion:
@hukkin: Thanks for the issue!

Yes, this is a very good idea, let's do it.

We already iterate over all `argparse` actions added by plugins [here](https://github.com/hukkin/mdformat/blob/f6ce9c95a374b7aafd15cab080a2c7a8db435984/src/mdformat/_cli.py#L297). This can be added in the same loop.

[This line](https://github.com/hukkin/mdformat/blob/f6ce9c95a374b7aafd15cab080a2c7a8db435984/src/mdformat/_cli.py#L284) shows how a warning can be pointed at the plugin. I think that's the way to do it here, too, except instead of a `DeprecationWarning` maybe just a `UserWarning`?

I think the most likely case to trigger the warning is `action="store_true"`  and `action="store_false"`, that you mentioned, so maybe the warning message can suggest  `action="store_const", const=<bool>` (and generally not using a non-None default, if TOML override is expected to work).

Is this something you want to work on?@KyleKing: Great! I would be happy to submit a PR@hukkin: Great!

I was wondering, since a `UserWarning` to my knowledge is visible to end users, it's annoying enough that no plugin developer wants to leave it that way, even if the behavior is otherwise precisely as intended by the developer (e.g. a CLI option that makes no sense in TOML). Almost might as well raise a hard error instead. We'd optimally have a warning with the same visibility as `DeprecationWarning`, i.e. it shows in test suites but not to users.

Maybe use `DeprecationWarning`, and either turn it into an error later, or have infinite deprecation period to avoid the issue with `UserWarning`?

Solution patch:
diff --git a/src/mdformat/_cli.py b/src/mdformat/_cli.py
index 0e13d3f..92fc92e 100644
--- a/src/mdformat/_cli.py
+++ b/src/mdformat/_cli.py
@@ -303,6 +303,16 @@ def make_arg_parser(
             plugin.add_cli_argument_group(group)
             for action in group._group_actions:
                 action.dest = f"plugin.{plugin_id}.{action.dest}"
+                if action.default not in {None, argparse.SUPPRESS}:
+                    import warnings
+
+                    plugin_file, plugin_line = get_source_file_and_line(plugin)
+                    warnings.warn_explicit(
+                        f"The `default` ({action.default!r}) for {action.option_strings!r} from the {plugin_id!r} plugin, will always override any value configured in TOML. The only supported CLI defaults are `None` or `argparse.SUPPRESS`. To resolve, consider refactoring to `.add_argument(..., default=None)` ",  # noqa: E501
+                        DeprecationWarning,
+                        filename=plugin_file,
+                        lineno=plugin_line,
+                    )
     return parser
 
 


Test patch:
diff --git a/tests/test_plugins.py b/tests/test_plugins.py
index 05623a8..1b596ee 100644
--- a/tests/test_plugins.py
+++ b/tests/test_plugins.py
@@ -272,6 +272,35 @@ dont_override_toml = 'dont override this with None if CLI opt not given'
     )
 
 
+def test_plugin_argument_warnings(monkeypatch, tmp_path):
+    """Test for warnings of plugin arguments that conflict with TOML."""
+
+    class ExamplePluginWithStoreTrue:
+        @staticmethod
+        def update_mdit(mdit: MarkdownIt):
+            pass
+
+        @staticmethod
+        def add_cli_argument_group(group: argparse._ArgumentGroup) -> None:
+            group.add_argument("--store-true", action="store_true")
+            group.add_argument("--store-false", action="store_false")
+            group.add_argument("--store-zero", default=0)
+            group.add_argument("--store-const", action="store_const", const=True)
+
+    monkeypatch.setitem(PARSER_EXTENSIONS, "table", ExamplePluginWithStoreTrue)
+    file_path = tmp_path / "test_markdown.md"
+    file_path.touch()
+
+    with patch.object(MDRenderer, "render", return_value=""):
+        with pytest.warns(DeprecationWarning) as warnings:
+            assert run([str(file_path)]) == 0
+
+    assert "--store-true" in str(warnings.pop().message)
+    assert "--store-false" in str(warnings.pop().message)
+    assert "--store-zero" in str(warnings.pop().message)
+    assert len(warnings) == 0
+
+
 def test_cli_options_group__no_toml(monkeypatch, tmp_path):
     """Test add_cli_argument_group plugin API with configuration only from
     CLI."""