From 7f9f70064c50bbb6024fafed4568e0693f5640b5 Mon Sep 17 00:00:00 2001 From: "Eric N. Vander Weele" Date: Sun, 29 Dec 2019 17:22:26 -0500 Subject: [PATCH 1/2] aggregator: Forward --config and --isolated options during aggregation This fixes a regression introduced in !346 to ensure that `--config` and `--isolated` are recognized in `aggregate_options()`. The regression manifested because `aggregate_options()` was relying on re-parsing `argv` to obtain the option values. However, !346 changed the preliminary parsing logic to only parse and "eat" what is necessary and forward along the options needed before all the configuration was loaded. This code path was overlooked because the tests in `test_aggregator()` were passing but the call from the `Application` object would never have these options in the remaining `argv` list to be passed long. --- src/flake8/api/legacy.py | 7 ++++++- src/flake8/main/application.py | 29 ++++++++++++++++++++++++---- src/flake8/options/aggregator.py | 17 +++++++++------- tests/integration/test_aggregator.py | 10 ++++------ tests/unit/test_legacy_api.py | 2 +- 5 files changed, 46 insertions(+), 19 deletions(-) diff --git a/src/flake8/api/legacy.py b/src/flake8/api/legacy.py index 2761163..fe3b405 100644 --- a/src/flake8/api/legacy.py +++ b/src/flake8/api/legacy.py @@ -39,7 +39,12 @@ def get_style_guide(**kwargs): config_finder, prelim_opts.config, prelim_opts.isolated ) application.register_plugin_options() - application.parse_configuration_and_cli(config_finder, remaining_args) + application.parse_configuration_and_cli( + config_finder, + prelim_opts.config, + prelim_opts.isolated, + remaining_args, + ) # We basically want application.initialize to be called but with these # options set instead before we make our formatter, notifier, internal # style guide and file checker manager. diff --git a/src/flake8/main/application.py b/src/flake8/main/application.py index 63db8e1..791d5af 100644 --- a/src/flake8/main/application.py +++ b/src/flake8/main/application.py @@ -169,17 +169,33 @@ class Application(object): self.check_plugins.register_plugin_versions(self.option_manager) self.formatting_plugins.register_options(self.option_manager) - def parse_configuration_and_cli(self, config_finder, argv): - # type: (config.ConfigFileFinder, List[str]) -> None + def parse_configuration_and_cli( + self, + config_finder, # type: config.ConfigFileFinder + config_file, # type: Optional[str] + ignore_config_files, # type: bool + argv, # type: List[str] + ): + # type: (...) -> None """Parse configuration files and the CLI options. :param config.ConfigFileFinder config_finder: The finder for finding and reading configuration files. + :param str config_file: + The optional configuraiton file to override all other configuration + files (i.e., the --config option). + :param bool ignore_config_files: + Determine whether to parse configuration files or not. (i.e., the + --isolated option). :param list argv: Command-line arguments passed in directly. """ self.options, self.args = aggregator.aggregate_options( - self.option_manager, config_finder, argv + self.option_manager, + config_finder, + config_file, + ignore_config_files, + argv, ) self.running_against_diff = self.options.diff @@ -325,7 +341,12 @@ class Application(object): config_finder, prelim_opts.config, prelim_opts.isolated ) self.register_plugin_options() - self.parse_configuration_and_cli(config_finder, remaining_args) + self.parse_configuration_and_cli( + config_finder, + prelim_opts.config, + prelim_opts.isolated, + remaining_args, + ) self.make_formatter() self.make_guide() self.make_file_checker_manager() diff --git a/src/flake8/options/aggregator.py b/src/flake8/options/aggregator.py index 719160a..58d1022 100644 --- a/src/flake8/options/aggregator.py +++ b/src/flake8/options/aggregator.py @@ -5,7 +5,7 @@ applies the user-specified command-line configuration on top of it. """ import argparse import logging -from typing import List, Tuple +from typing import List, Optional, Tuple from flake8.options import config from flake8.options.manager import OptionManager @@ -16,6 +16,8 @@ LOG = logging.getLogger(__name__) def aggregate_options( manager, # type: OptionManager config_finder, # type: config.ConfigFileFinder + cli_config, # type: Optional[str] + isolated, # type: bool argv, # type: List[str] ): # type: (...) -> Tuple[argparse.Namespace, List[str]] """Aggregate and merge CLI and config file options. @@ -24,6 +26,12 @@ def aggregate_options( The instance of the OptionManager that we're presently using. :param flake8.options.config.ConfigFileFinder config_finder: The config file finder to use. + :param str cli_config: + Value of --config when specified at the command-line. Overrides + all other config files. + :param bool isolated: + Determines if we should parse configuration files at all or not. + If running in isolated mode, we ignore all configuration files :param list argv: The list of remaining command-line argumentsthat were unknown during preliminary option parsing to pass to ``manager.parse_args``. @@ -35,9 +43,6 @@ def aggregate_options( """ # Get defaults from the option parser default_values, _ = manager.parse_args([]) - # Get original CLI values so we can find additional config file paths and - # see if --config was specified. - original_values, _ = manager.parse_args(argv) # Make our new configuration file mergerator config_parser = config.MergedConfigParser( @@ -45,9 +50,7 @@ def aggregate_options( ) # Get the parsed config - parsed_config = config_parser.parse( - original_values.config, original_values.isolated - ) + parsed_config = config_parser.parse(cli_config, isolated) # Extend the default ignore value with the extended default ignore list, # registered by plugins. diff --git a/tests/integration/test_aggregator.py b/tests/integration/test_aggregator.py index d2c0133..18ce5e8 100644 --- a/tests/integration/test_aggregator.py +++ b/tests/integration/test_aggregator.py @@ -28,13 +28,12 @@ def optmanager(): def test_aggregate_options_with_config(optmanager): """Verify we aggregate options and config values appropriately.""" - arguments = ['flake8', '--config', CLI_SPECIFIED_CONFIG, '--select', + arguments = ['flake8', '--select', 'E11,E34,E402,W,F', '--exclude', 'tests/*'] config_finder = config.ConfigFileFinder('flake8', []) options, args = aggregator.aggregate_options( - optmanager, config_finder, arguments) + optmanager, config_finder, CLI_SPECIFIED_CONFIG, False, arguments) - assert options.config == CLI_SPECIFIED_CONFIG assert options.select == ['E11', 'E34', 'E402', 'W', 'F'] assert options.ignore == ['E123', 'W234', 'E111'] assert options.exclude == [os.path.abspath('tests/*')] @@ -42,14 +41,13 @@ def test_aggregate_options_with_config(optmanager): def test_aggregate_options_when_isolated(optmanager): """Verify we aggregate options and config values appropriately.""" - arguments = ['flake8', '--isolated', '--select', 'E11,E34,E402,W,F', + arguments = ['flake8', '--select', 'E11,E34,E402,W,F', '--exclude', 'tests/*'] config_finder = config.ConfigFileFinder('flake8', []) optmanager.extend_default_ignore(['E8']) options, args = aggregator.aggregate_options( - optmanager, config_finder, arguments) + optmanager, config_finder, None, True, arguments) - assert options.isolated is True assert options.select == ['E11', 'E34', 'E402', 'W', 'F'] assert sorted(options.ignore) == [ 'E121', 'E123', 'E126', 'E226', 'E24', 'E704', 'E8', 'W503', 'W504', diff --git a/tests/unit/test_legacy_api.py b/tests/unit/test_legacy_api.py index 8f6045e..d3b9eb5 100644 --- a/tests/unit/test_legacy_api.py +++ b/tests/unit/test_legacy_api.py @@ -34,7 +34,7 @@ def test_get_style_guide(): mockedapp.find_plugins.assert_called_once_with(config_finder, None, False) mockedapp.register_plugin_options.assert_called_once_with() mockedapp.parse_configuration_and_cli.assert_called_once_with( - config_finder, []) + config_finder, None, False, []) mockedapp.make_formatter.assert_called_once_with() mockedapp.make_guide.assert_called_once_with() mockedapp.make_file_checker_manager.assert_called_once_with() From 738c8490ec56fc364f4229df2c7c9adca8d1d4f2 Mon Sep 17 00:00:00 2001 From: "Eric N. Vander Weele" Date: Sun, 29 Dec 2019 18:40:09 -0500 Subject: [PATCH 2/2] tests: Add integration tests for `--config` and `--isolated` Prevent regressions by adding integration tests to ensure that these options are passed through to `aggregator.aggregate_options()`. --- tests/integration/test_main.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/integration/test_main.py b/tests/integration/test_main.py index f3ace53..42ad76c 100644 --- a/tests/integration/test_main.py +++ b/tests/integration/test_main.py @@ -167,3 +167,31 @@ def test_obtaining_args_from_sys_argv_when_not_explicity_provided(capsys): out, err = capsys.readouterr() assert out.startswith('usage: flake8 [options] file file ...\n') assert err == '' + + +def test_cli_config_option_respected(tmp_path): + """Test --config is used.""" + config = tmp_path / "flake8.ini" + config.write_text(u"""\ +[flake8] +ignore = F401 +""") + + py_file = tmp_path / "t.py" + py_file.write_text(u"import os\n") + + _call_main(["--config", str(config), str(py_file)]) + + +def test_cli_isolated_overrides_config_option(tmp_path): + """Test --isolated overrides --config.""" + config = tmp_path / "flake8.ini" + config.write_text(u"""\ +[flake8] +ignore = F401 +""") + + py_file = tmp_path / "t.py" + py_file.write_text(u"import os\n") + + _call_main(["--isolated", "--config", str(config), str(py_file)], retv=1)