From 2a5c2bb6969046b607bbb44bf5c7d2c774768d7c Mon Sep 17 00:00:00 2001 From: "Eric N. Vander Weele" Date: Fri, 25 Oct 2019 15:01:30 -0400 Subject: [PATCH 1/8] options: Split-out registration of preliminary options This is in preparation for having separate `ArgumentParser`s for preliminary and the remaining options. --- src/flake8/main/application.py | 1 + src/flake8/main/options.py | 98 +++++++++++++++------------- tests/integration/test_aggregator.py | 1 + 3 files changed, 56 insertions(+), 44 deletions(-) diff --git a/src/flake8/main/application.py b/src/flake8/main/application.py index 60b0c5c..5037293 100644 --- a/src/flake8/main/application.py +++ b/src/flake8/main/application.py @@ -50,6 +50,7 @@ class Application(object): self.option_manager = manager.OptionManager( prog="flake8", version=flake8.__version__ ) + options.register_preliminary_options(self.option_manager) options.register_default_options(self.option_manager) #: The instance of :class:`flake8.options.config.ConfigFileFinder` self.config_finder = None # type: config.ConfigFileFinder diff --git a/src/flake8/main/options.py b/src/flake8/main/options.py index 101bd1a..ad37660 100644 --- a/src/flake8/main/options.py +++ b/src/flake8/main/options.py @@ -6,12 +6,65 @@ from flake8.main import debug from flake8.main import vcs +def register_preliminary_options(option_manager): + """Register the preliminary options on our OptionManager. + + The preliminary options include: + + - ``-v``/``--verbose`` + - ``--output-file`` + - ``--append-config`` + - ``--config`` + - ``--isolated`` + """ + add_option = option_manager.add_option + + add_option( + "-v", + "--verbose", + default=0, + action="count", + help="Print more information about what is happening in flake8." + " This option is repeatable and will increase verbosity each " + "time it is repeated.", + ) + + add_option( + "--output-file", default=None, help="Redirect report to a file." + ) + + # Config file options + + add_option( + "--append-config", + action="append", + help="Provide extra config files to parse in addition to the files " + "found by Flake8 by default. These files are the last ones read " + "and so they take the highest precedence when multiple files " + "provide the same option.", + ) + + add_option( + "--config", + default=None, + help="Path to the config file that will be the authoritative config " + "source. This will cause Flake8 to ignore all other " + "configuration files.", + ) + + add_option( + "--isolated", + default=False, + action="store_true", + help="Ignore all configuration files.", + ) + + def register_default_options(option_manager): """Register the default options on our OptionManager. The default options include: - - ``-v``/``--verbose`` - ``-q``/``--quiet`` - ``--count`` - ``--diff`` @@ -32,26 +85,13 @@ def register_default_options(option_manager): - ``--enable-extensions`` - ``--exit-zero`` - ``-j``/``--jobs`` - - ``--output-file`` - ``--tee`` - - ``--append-config`` - - ``--config`` - - ``--isolated`` - ``--benchmark`` - ``--bug-report`` """ add_option = option_manager.add_option # pep8 options - add_option( - "-v", - "--verbose", - default=0, - action="count", - help="Print more information about what is happening in flake8." - " This option is repeatable and will increase verbosity each " - "time it is repeated.", - ) add_option( "-q", "--quiet", @@ -257,10 +297,6 @@ def register_default_options(option_manager): " (Default: %(default)s)", ) - add_option( - "--output-file", default=None, help="Redirect report to a file." - ) - add_option( "--tee", default=False, @@ -269,32 +305,6 @@ def register_default_options(option_manager): help="Write to stdout and output-file.", ) - # Config file options - - add_option( - "--append-config", - action="append", - help="Provide extra config files to parse in addition to the files " - "found by Flake8 by default. These files are the last ones read " - "and so they take the highest precedence when multiple files " - "provide the same option.", - ) - - add_option( - "--config", - default=None, - help="Path to the config file that will be the authoritative config " - "source. This will cause Flake8 to ignore all other " - "configuration files.", - ) - - add_option( - "--isolated", - default=False, - action="store_true", - help="Ignore all configuration files.", - ) - # Benchmarking add_option( diff --git a/tests/integration/test_aggregator.py b/tests/integration/test_aggregator.py index a0e0940..5af24ef 100644 --- a/tests/integration/test_aggregator.py +++ b/tests/integration/test_aggregator.py @@ -18,6 +18,7 @@ def optmanager(): prog='flake8', version='3.0.0', ) + options.register_preliminary_options(option_manager) options.register_default_options(option_manager) return option_manager From 1d7558f7da77d67272352d6ab8ca8a476cf80411 Mon Sep 17 00:00:00 2001 From: "Eric N. Vander Weele" Date: Fri, 25 Oct 2019 15:01:30 -0400 Subject: [PATCH 2/8] optmanager: Inherit options from parent argument parsers Allow for including options from parent `argparse.ArgumentParser` objects in preparation of splitting out the handling of preliminary options from the `OptionManager`. --- src/flake8/options/manager.py | 16 +++++++++++++--- tests/unit/test_option_manager.py | 17 +++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/src/flake8/options/manager.py b/src/flake8/options/manager.py index a7f678d..def4c96 100644 --- a/src/flake8/options/manager.py +++ b/src/flake8/options/manager.py @@ -338,8 +338,12 @@ class OptionManager(object): """Manage Options and OptionParser while adding post-processing.""" def __init__( - self, prog, version, usage="%(prog)s [options] file file ..." - ): # type: (str, str, str) -> None + self, + prog, + version, + usage="%(prog)s [options] file file ...", + parents=None, + ): # type: (str, str, str, Optional[List[argparse.ArgumentParser]]) -> None # noqa: E501 """Initialize an instance of an OptionManager. :param str prog: @@ -348,9 +352,15 @@ class OptionManager(object): Version string for the program. :param str usage: Basic usage string used by the OptionParser. + :param argparse.ArgumentParser parents: + A list of ArgumentParser objects whose arguments should also be + included. """ + if parents is None: + parents = [] + self.parser = argparse.ArgumentParser( - prog=prog, usage=usage + prog=prog, usage=usage, parents=parents ) # type: argparse.ArgumentParser self._current_group = None # type: Optional[argparse._ArgumentGroup] self.version_action = cast( diff --git a/tests/unit/test_option_manager.py b/tests/unit/test_option_manager.py index b384a31..b97a9a6 100644 --- a/tests/unit/test_option_manager.py +++ b/tests/unit/test_option_manager.py @@ -22,6 +22,23 @@ def test_option_manager_creates_option_parser(optmanager): assert isinstance(optmanager.parser, argparse.ArgumentParser) +def test_option_manager_including_parent_options(): + """Verify parent options are included in the parsed options.""" + # GIVEN + parent_parser = argparse.ArgumentParser(add_help=False) + parent_parser.add_argument('--parent') + + # WHEN + optmanager = manager.OptionManager( + prog='flake8', + version=TEST_VERSION, + parents=[parent_parser]) + option, _ = optmanager.parse_args(['--parent', 'foo']) + + # THEN + assert option.parent == 'foo' + + def test_parse_args_forwarding_default_values(optmanager): """Verify default provided values are present in the final result.""" namespace = argparse.Namespace(foo='bar') From a90200353edb3d605e16e9ea852fe33f22f8d7a6 Mon Sep 17 00:00:00 2001 From: "Eric N. Vander Weele" Date: Fri, 25 Oct 2019 15:01:30 -0400 Subject: [PATCH 3/8] application: Register preliminary options on a separate argument parser We introduce a new `ArgumentParser` for registering the preliminary options to be inherited by the `Application.option_manager`. The next step will be to use the `Application.prelim_arg_parser` for parsing and handling preliminary options and arguments. Note that we prevent the preliminary parser from handling `-h/--help` and defer to that to the primary parser. --- src/flake8/main/application.py | 9 +++++++-- src/flake8/main/options.py | 16 +++++++++------- tests/integration/test_aggregator.py | 5 ++++- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/flake8/main/application.py b/src/flake8/main/application.py index 5037293..e183351 100644 --- a/src/flake8/main/application.py +++ b/src/flake8/main/application.py @@ -45,12 +45,17 @@ class Application(object): self.program = program #: The version of the program being run self.version = version + #: The prelimary argument parser for handling options required for + #: obtaining and parsing the configuration file. + self.prelim_arg_parser = argparse.ArgumentParser(add_help=False) + options.register_preliminary_options(self.prelim_arg_parser) #: The instance of :class:`flake8.options.manager.OptionManager` used #: to parse and handle the options and arguments passed by the user self.option_manager = manager.OptionManager( - prog="flake8", version=flake8.__version__ + prog="flake8", + version=flake8.__version__, + parents=[self.prelim_arg_parser], ) - options.register_preliminary_options(self.option_manager) options.register_default_options(self.option_manager) #: The instance of :class:`flake8.options.config.ConfigFileFinder` self.config_finder = None # type: config.ConfigFileFinder diff --git a/src/flake8/main/options.py b/src/flake8/main/options.py index ad37660..ba1f1c2 100644 --- a/src/flake8/main/options.py +++ b/src/flake8/main/options.py @@ -1,4 +1,5 @@ """Contains the logic for all of the default options for Flake8.""" +import argparse import functools from flake8 import defaults @@ -6,7 +7,8 @@ from flake8.main import debug from flake8.main import vcs -def register_preliminary_options(option_manager): +def register_preliminary_options(parser): + # type: (argparse.ArgumentParser) -> None """Register the preliminary options on our OptionManager. The preliminary options include: @@ -17,9 +19,9 @@ def register_preliminary_options(option_manager): - ``--config`` - ``--isolated`` """ - add_option = option_manager.add_option + add_argument = parser.add_argument - add_option( + add_argument( "-v", "--verbose", default=0, @@ -29,13 +31,13 @@ def register_preliminary_options(option_manager): "time it is repeated.", ) - add_option( + add_argument( "--output-file", default=None, help="Redirect report to a file." ) # Config file options - add_option( + add_argument( "--append-config", action="append", help="Provide extra config files to parse in addition to the files " @@ -44,7 +46,7 @@ def register_preliminary_options(option_manager): "provide the same option.", ) - add_option( + add_argument( "--config", default=None, help="Path to the config file that will be the authoritative config " @@ -52,7 +54,7 @@ def register_preliminary_options(option_manager): "configuration files.", ) - add_option( + add_argument( "--isolated", default=False, action="store_true", diff --git a/tests/integration/test_aggregator.py b/tests/integration/test_aggregator.py index 5af24ef..d2c0133 100644 --- a/tests/integration/test_aggregator.py +++ b/tests/integration/test_aggregator.py @@ -1,4 +1,5 @@ """Test aggregation of config files and command-line options.""" +import argparse import os import pytest @@ -14,11 +15,13 @@ CLI_SPECIFIED_CONFIG = 'tests/fixtures/config_files/cli-specified.ini' @pytest.fixture def optmanager(): """Create a new OptionManager.""" + prelim_parser = argparse.ArgumentParser(add_help=False) + options.register_preliminary_options(prelim_parser) option_manager = manager.OptionManager( prog='flake8', version='3.0.0', + parents=[prelim_parser], ) - options.register_preliminary_options(option_manager) options.register_default_options(option_manager) return option_manager From 7f46990f4b1dcd59e8c593538b37578cbf9f3d77 Mon Sep 17 00:00:00 2001 From: "Eric N. Vander Weele" Date: Fri, 25 Oct 2019 15:01:30 -0400 Subject: [PATCH 4/8] application: Change to `argparse.ArgumentParser` for preliminary parsing Now that preliminary options are registered with the preliminary parser on the `Application`, leverage that for parsing known options. This important change removes the `Application.option_manager` from being responsible for pre-configuration parsing and the workarounds needed in the `Application.parse_preliminary_options_and_args()` to account for the fact that `Application.option_manager` was aware of *all* options, not just the options necessary for pre-configuration loading. A following commit will address removing these workarounds. --- src/flake8/main/application.py | 2 +- tests/unit/test_application.py | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/flake8/main/application.py b/src/flake8/main/application.py index e183351..0f86844 100644 --- a/src/flake8/main/application.py +++ b/src/flake8/main/application.py @@ -138,7 +138,7 @@ class Application(object): except ValueError: pass - opts, args = self.option_manager.parse_known_args(args) + opts, args = self.prelim_arg_parser.parse_known_args(args) # parse_known_args includes unknown options as args args = [a for a in args if not a.startswith("-")] return opts, args diff --git a/tests/unit/test_application.py b/tests/unit/test_application.py index 09edfe4..15bc54d 100644 --- a/tests/unit/test_application.py +++ b/tests/unit/test_application.py @@ -93,9 +93,10 @@ def test_returns_specified_plugin(application): def test_prelim_opts_args(application): """Verify we get sensible prelim opts and args.""" opts, args = application.parse_preliminary_options_and_args( - ['flake8', '--foo', '--verbose', 'src', 'setup.py', '--statistics']) + ['--foo', '--verbose', 'src', 'setup.py', '--statistics']) - assert opts.statistics + assert not hasattr(opts, 'foo') + assert not hasattr(opts, 'statistics') assert opts.verbose assert args == ['src', 'setup.py'] From 2260f5362ef3d136e4233d65735641d9714f9ffc Mon Sep 17 00:00:00 2001 From: "Eric N. Vander Weele" Date: Fri, 25 Oct 2019 15:01:30 -0400 Subject: [PATCH 5/8] application: Keep unknown options in the unknown argument list Positional arguments aren't necessary for determining where to load configuration anymore and is safe to keep both options and arguments to be forwarded for later parsing after configuration is loaded. --- src/flake8/main/application.py | 5 +---- tests/unit/test_application.py | 4 +--- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/flake8/main/application.py b/src/flake8/main/application.py index 0f86844..2354425 100644 --- a/src/flake8/main/application.py +++ b/src/flake8/main/application.py @@ -138,10 +138,7 @@ class Application(object): except ValueError: pass - opts, args = self.prelim_arg_parser.parse_known_args(args) - # parse_known_args includes unknown options as args - args = [a for a in args if not a.startswith("-")] - return opts, args + return self.prelim_arg_parser.parse_known_args(args) def exit(self): # type: () -> None diff --git a/tests/unit/test_application.py b/tests/unit/test_application.py index 15bc54d..edf853b 100644 --- a/tests/unit/test_application.py +++ b/tests/unit/test_application.py @@ -95,10 +95,8 @@ def test_prelim_opts_args(application): opts, args = application.parse_preliminary_options_and_args( ['--foo', '--verbose', 'src', 'setup.py', '--statistics']) - assert not hasattr(opts, 'foo') - assert not hasattr(opts, 'statistics') assert opts.verbose - assert args == ['src', 'setup.py'] + assert args == ['--foo', 'src', 'setup.py', '--statistics'] def test_prelim_opts_handles_empty(application): From b9c8c3e118492a5201c444434a3000436a007926 Mon Sep 17 00:00:00 2001 From: "Eric N. Vander Weele" Date: Fri, 25 Oct 2019 15:01:30 -0400 Subject: [PATCH 6/8] application: Ensure `-h/--help` is unknown during preliminary parsing Now that the preliminary parser is being used, we can remove needing to prune out `-h` and `--help` from the copied `args` list. --- src/flake8/main/application.py | 8 -------- tests/unit/test_application.py | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/flake8/main/application.py b/src/flake8/main/application.py index 2354425..5e61fb0 100644 --- a/src/flake8/main/application.py +++ b/src/flake8/main/application.py @@ -129,14 +129,6 @@ class Application(object): args.remove("--version") except ValueError: pass - try: - args.remove("--help") - except ValueError: - pass - try: - args.remove("-h") - except ValueError: - pass return self.prelim_arg_parser.parse_known_args(args) diff --git a/tests/unit/test_application.py b/tests/unit/test_application.py index edf853b..a3d9bba 100644 --- a/tests/unit/test_application.py +++ b/tests/unit/test_application.py @@ -99,6 +99,20 @@ def test_prelim_opts_args(application): assert args == ['--foo', 'src', 'setup.py', '--statistics'] +def test_prelim_opts_ignore_help(application): + """Verify -h/--help is not handled.""" + # GIVEN + + # WHEN + _, args = application.parse_preliminary_options_and_args([ + '--help', + '-h', + ]) + + # THEN + assert args == ['--help', '-h'] + + def test_prelim_opts_handles_empty(application): """Verify empty argv lists are handled correctly.""" irrelevant_args = ['myexe', '/path/to/foo'] From 4151ae0e532c9f5660b08747ac7f1b8411aada19 Mon Sep 17 00:00:00 2001 From: "Eric N. Vander Weele" Date: Fri, 25 Oct 2019 15:01:30 -0400 Subject: [PATCH 7/8] application: Ensure `--version` is unknown during preliminary parsing Now that the preliminary parser is being used, we can remove needing to prune out `--version` and copying the original `argv` list. --- src/flake8/main/application.py | 16 +--------------- tests/unit/test_application.py | 4 ++-- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/src/flake8/main/application.py b/src/flake8/main/application.py index 5e61fb0..8290068 100644 --- a/src/flake8/main/application.py +++ b/src/flake8/main/application.py @@ -116,21 +116,7 @@ class Application(object): :rtype: (argparse.Namespace, list) """ - # We haven't found or registered our plugins yet, so let's defer - # printing the version until we aggregate options from config files - # and the command-line. First, let's clone our arguments on the CLI, - # then we'll attempt to remove ``--version`` so that we can avoid - # triggering the "version" action in argparse. If it's not there, we - # do not need to worry and we can continue. If it is, we successfully - # defer printing the version until just a little bit later. - # Similarly we have to defer printing the help text until later. - args = argv[:] - try: - args.remove("--version") - except ValueError: - pass - - return self.prelim_arg_parser.parse_known_args(args) + return self.prelim_arg_parser.parse_known_args(argv) def exit(self): # type: () -> None diff --git a/tests/unit/test_application.py b/tests/unit/test_application.py index a3d9bba..9d4debc 100644 --- a/tests/unit/test_application.py +++ b/tests/unit/test_application.py @@ -93,10 +93,10 @@ def test_returns_specified_plugin(application): def test_prelim_opts_args(application): """Verify we get sensible prelim opts and args.""" opts, args = application.parse_preliminary_options_and_args( - ['--foo', '--verbose', 'src', 'setup.py', '--statistics']) + ['--foo', '--verbose', 'src', 'setup.py', '--statistics', '--version']) assert opts.verbose - assert args == ['--foo', 'src', 'setup.py', '--statistics'] + assert args == ['--foo', 'src', 'setup.py', '--statistics', '--version'] def test_prelim_opts_ignore_help(application): From 964b3a9c21997851ef1e3305854545229a1e2ec5 Mon Sep 17 00:00:00 2001 From: "Eric N. Vander Weele" Date: Fri, 25 Oct 2019 15:20:06 -0400 Subject: [PATCH 8/8] application: Forward remaining unknown arguments to final CLI parsing Now that `parse_preliminary_options_and_args()` ignores unknown options and arguments, forward the remaining unknown arguments to the main CLI and configuration method to be consumed. This prevents re-parsing the entire `argv` list again by forwarding the remaining arguments left to be consumed. --- src/flake8/api/legacy.py | 4 ++-- src/flake8/main/application.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/flake8/api/legacy.py b/src/flake8/api/legacy.py index 1056fe9..e530088 100644 --- a/src/flake8/api/legacy.py +++ b/src/flake8/api/legacy.py @@ -28,14 +28,14 @@ def get_style_guide(**kwargs): :class:`StyleGuide` """ application = app.Application() - prelim_opts, prelim_args = application.parse_preliminary_options_and_args( + prelim_opts, remaining_args = application.parse_preliminary_options_and_args( # noqa: E501 [] ) flake8.configure_logging(prelim_opts.verbose, prelim_opts.output_file) application.make_config_finder(prelim_opts.append_config) application.find_plugins(prelim_opts.config, prelim_opts.isolated) application.register_plugin_options() - application.parse_configuration_and_cli([]) + application.parse_configuration_and_cli(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 8290068..9555202 100644 --- a/src/flake8/main/application.py +++ b/src/flake8/main/application.py @@ -338,14 +338,14 @@ class Application(object): """ # NOTE(sigmavirus24): When updating this, make sure you also update # our legacy API calls to these same methods. - prelim_opts, prelim_args = self.parse_preliminary_options_and_args( + prelim_opts, remaining_args = self.parse_preliminary_options_and_args( argv ) flake8.configure_logging(prelim_opts.verbose, prelim_opts.output_file) self.make_config_finder(prelim_opts.append_config) self.find_plugins(prelim_opts.config, prelim_opts.isolated) self.register_plugin_options() - self.parse_configuration_and_cli(argv) + self.parse_configuration_and_cli(remaining_args) self.make_formatter() self.make_guide() self.make_file_checker_manager()