option_manager.add_option ignores action when reading from config

[jakkdl]

how did you install flake8?

pip install flake8

also when cloned from github master

unmodified output of flake8 --bug-report

{
  "platform": {
    "python_implementation": "CPython",
    "python_version": "3.10.8",
    "system": "Linux"
  },
  "plugins": [
    {
      "plugin": "mccabe",
      "version": "0.7.0"
    },
    {
      "plugin": "pycodestyle",
      "version": "2.10.0"
    },
    {
      "plugin": "pyflakes",
      "version": "3.0.1"
    }
  ],
  "version": "6.0.0"
}

describe the problem

what I expected to happen

my argparse.Action to be called regardless of if the parameter is specified on the command line or in the config

sample code

I implemented a test that reproduces the error. The first one passes, the second one doesn't.

from collections.abc import Sequence
from flake8.options import config

# always sets the value to bar regardless of what `values` is
class MyAction(argparse.Action):
    def __call__(
        self,
        parser: argparse.ArgumentParser,
        namespace: argparse.Namespace,
        values: Sequence[str] | None,
        option_string: str | None = None,
    ) -> None:
        setattr(namespace, self.dest, "bar")


def test_action_cmdline(optmanager):
    optmanager.add_option(
        "--my-option",
        parse_from_config=True,
        required=False,
        action=MyAction,
    )
    options = optmanager.parse_args(["--my-option", "foo"])
    assert options.my_option == 'bar'

def test_action_config(tmpdir):
    tmpdir.join("setup.cfg").write("[flake8]\nmy-option=foo\n")

    with tmpdir.as_cwd():
        cfg, cfg_dir = config.load_config(None, [], isolated=False)

    assert cfg.get("flake8", "my-option") == "bar"

commands ran

$ pytest -k test_action_ 
===================================== test session starts =====================================
platform linux -- Python 3.10.8, pytest-7.2.0, pluggy-1.0.0
rootdir: /home/h/Git/flake8, configfile: pytest.ini
collected 465 items / 463 deselected / 2 selected                                             

tests/unit/test_option_manager.py .F                                                    [100%]

========================================== FAILURES ===========================================
_____________________________________ test_action_config ______________________________________

tmpdir = local('/tmp/pytest-of-h/pytest-62/test_action_config0')

    def test_action_config(tmpdir):
        tmpdir.join("setup.cfg").write("[flake8]\nmy-option=foo\n")
    
        with tmpdir.as_cwd():
            cfg, cfg_dir = config.load_config(None, [], isolated=False)
    
>       assert cfg.get("flake8", "my-option") == "bar"
E       AssertionError: assert 'foo' == 'bar'
E         - bar
E         + foo

tests/unit/test_option_manager.py:243: AssertionError
========================= 1 failed, 1 passed, 463 deselected in 0.26s =========================

The value is foo, which it would be if there's no action specified, but it completely sidesteps calling the action completely.

[sigmavirus24]

While we rely on argparse under our abstraction, we don't document support for actions. https://flake8.pycqa.org/en/latest/plugin-development/plugin-parameters.html#registering-options

It wouldn't be easy to add support for that since we don't want to be tied too closely to any argument parsing backend. Beyond that, parsing config files is separate from CLI options. I don't even think argparse gives us a good way to trigger your action unfortunately

[jakkdl]

hm, that's a shame. But if deemed not worth supporting you should maybe give a warning if both action and parse_from_config is specified, or at least document it in help(option_manager.add_option).

[asottile]

~generally you want to use type for this -- can you provide a concrete use case instead of an abstract one?

[Zac-HD]

We ran into this in flake8-trio; I think we can just switch to type=function_to_parse_it for our use-case.

Might be useful to warn and eventually error on passing action= though with a message recommending type=; since it does currently work via the CLI but not via config files we just didn't realize that it wasn't supported.

[jakkdl]

the type parameter is also silently ignored if you pass comma_separated_list=True (regardless of if read from config or CLI). I expected it to be usable to validate the parameter value, but that can be done after the fact, so that probably warrants an emitted warning as well.