fix: keep global options given before the subcommand

tabcmd/commands/auth/session.py

@@ -81,6 +81,8 @@ def _update_session_data(self, args):
         self.no_prompt = args.no_prompt  # have to set this on every call?
         self.certificate = args.certificate or self.certificate
         self.no_certcheck = args.no_certcheck  # have to set this on every call?
+        if args.no_proxy and args.proxy:
+            Errors.exit_with_error(self.logger, _("session.errors.proxy_conflict"))
         self.no_proxy = args.no_proxy  # have to set this on every call?
         self.proxy = args.proxy or self.proxy
         self.timeout = self.timeout_as_integer(self.logger, args.timeout, self.timeout)

tabcmd/execution/parent_parser.py

@@ -21,7 +21,10 @@
 """
 
 
-def parent_parser_with_global_options():
+def parent_parser_with_global_options(suppress_defaults: bool = False):
+    """Global options are accepted both before and after the subcommand.
+    The copy used by the subcommand parsers must not set defaults: argparse would otherwise
+    overwrite a value given before the subcommand with the default."""
     parser = argparse.ArgumentParser(usage=argparse.SUPPRESS, add_help=False)
     parser._optionals.title = _("tabcmdparser.global.behaviors")
 
@@ -139,6 +142,9 @@ def parent_parser_with_global_options():
         metavar="<PAGE_SIZE>",
         help=_("tabcmd.global.help.page_size"),
     )
+    if suppress_defaults:
+        for action in parser._actions:
+            action.default = argparse.SUPPRESS
     return parser
 
 
@@ -148,6 +154,7 @@ class ParentParser:
 
     def __init__(self):
         self.global_options = parent_parser_with_global_options()
+        self.subcommand_global_options = parent_parser_with_global_options(suppress_defaults=True)
         self.root = argparse.ArgumentParser(
             prog="tabcmd",
             description="tabcmd <command>      -- " + _("tabcmd.howto"),
@@ -173,7 +180,7 @@ def connect_commands(self):
 
     def include(self, command):
         additional_parser = self.subparsers.add_parser(
-            command.name, help=command.description, parents=[self.global_options]
+            command.name, help=command.description, parents=[self.subcommand_global_options]
         )
         additional_parser._optionals.title = _("tabcmdparser.global.connections")
         # This line is where we actually set each parser to call the correct command
@@ -184,7 +191,7 @@ def include(self, command):
     # Help isn't added like the others because it has to have access to the rest, to get their args
     def include_help(self):
         additional_parser = self.subparsers.add_parser(
-            "help", help=_("tabcmdparser.help.description"), parents=[self.global_options]
+            "help", help=_("tabcmdparser.help.description"), parents=[self.subcommand_global_options]
         )
         additional_parser._optionals.title = _("tabcmdparser.global.connections")
         additional_parser.set_defaults(func=Help(self))

tabcmd/locales/en/tabcmd_messages_en.properties

@@ -122,6 +122,7 @@ session.auto_site_login=Auto-sign in to site: {0}
 session.connecting=Connecting to the server...
 session.continuing_session=Continuing previous session
 session.errors.missing_arguments=Cannot sign in because of missing arguments: {0}
+session.errors.proxy_conflict=Cannot use --proxy together with --no-proxy. Pass only one of them.
 session.errors.password_file_not_found=Password file not found: '{0}'. Check the path passed to --password-file.
 session.errors.token_file_not_found=Token file not found: '{0}'. Check the path passed to --token-file.
 session.errors.script_no_password=Tabcmd was run from a script or IDE but no password was provided. A password must be provided as an argument to tabcmd in order to log in.

tests/commands/test_session.py

@@ -199,6 +199,31 @@ def test__create_new_username_credential_succeeds_from_args(self, mock_pass):
         auth = active_session._create_new_credential(test_args.password, Session.PASSWORD_CRED_TYPE)
 
 
+class ProxyOptionTests(unittest.TestCase):
+    def test_proxy_with_no_proxy_is_rejected(self):
+        test_args = Namespace(**vars(args_to_mock))
+        test_args.proxy = "host:1"
+        test_args.no_proxy = True
+        with self.assertRaises(SystemExit):
+            Session()._update_session_data(test_args)
+
+    def test_proxy_alone_is_accepted(self):
+        test_args = Namespace(**vars(args_to_mock))
+        test_args.proxy = "host:1"
+        test_args.no_proxy = False
+        mock_session = Session()
+        mock_session._update_session_data(test_args)
+        assert mock_session.proxy == "host:1"
+
+    def test_no_proxy_alone_is_accepted(self):
+        test_args = Namespace(**vars(args_to_mock))
+        test_args.proxy = None
+        test_args.no_proxy = True
+        mock_session = Session()
+        mock_session._update_session_data(test_args)
+        assert mock_session.no_proxy is True
+
+
 class PromptingTests(unittest.TestCase):
     def test_show_prompt_if_user_didnt_say(self):
         test_args = Namespace(**vars(args_to_mock))

tests/parsers/test_global_options_position.py

@@ -0,0 +1,150 @@
+import argparse
+import unittest
+from unittest import mock
+
+from tabcmd.commands.auth.login_command import LoginCommand
+from tabcmd.execution.parent_parser import ParentParser, parent_parser_with_global_options
+from .common_setup import *
+
+commandname: str = "login"
+
+
+@mock.patch("sys.argv", None)
+class GlobalOptionsPositionTest(ParserTest):
+    """Global options must mean the same thing before and after the subcommand."""
+
+    @classmethod
+    def setUpClass(cls):
+        cls.parser_under_test = initialize_test_pieces(commandname, LoginCommand)
+
+    def test_global_options_before_command_are_kept(self):
+        args = self.parser_under_test.parse_args(
+            ["--server", "the-server", "--site", "the-site", "--token-name", "n", "--language", "de", commandname]
+        )
+        assert args.server == "the-server", args
+        assert args.site_name == "the-site", args
+        assert args.token_name == "n", args
+        assert args.language == "de", args
+
+    def test_global_options_after_command_are_kept(self):
+        args = self.parser_under_test.parse_args(
+            [commandname, "--server", "the-server", "--site", "the-site", "--token-name", "n", "--language", "de"]
+        )
+        assert args.server == "the-server", args
+        assert args.site_name == "the-site", args
+        assert args.token_name == "n", args
+        assert args.language == "de", args
+
+    def test_option_after_command_wins_over_option_before(self):
+        args = self.parser_under_test.parse_args(["--server", "first", commandname, "--server", "second"])
+        assert args.server == "second", args
+
+    def test_defaults_apply_when_option_not_given(self):
+        args = self.parser_under_test.parse_args([commandname])
+        assert args.server is None, args
+        assert args.language is None, args
+        assert args.site_name == "", args
+        assert args.logging_level == "INFO", args
+
+
+# every global option, with a value where it takes one; each must mean the same thing in either position
+GLOBAL_OPTIONS = [
+    ["-s", "the-server"],
+    ["-t", "the-site"],
+    ["-u", "the-user"],
+    ["--token-name", "the-token-name"],
+    ["-p", "the-password"],
+    ["--token-value", "the-token-value"],
+    ["--password-file", "the-file"],
+    ["--token-file", "the-file"],
+    ["--no-prompt"],
+    ["-c", "the-cert"],
+    ["--no-certcheck"],
+    ["--no-cookie"],
+    ["-x", "host:1"],
+    ["--no-proxy"],
+    ["--timeout", "30"],
+    ["--continue-if-exists"],
+    ["--country", "DE"],
+    ["--language", "DE"],
+    ["-l", "debug"],
+    ["--query-page-size", "5"],
+]
+
+# commands that parse with the given arguments and nothing else
+COMMANDS = [["login"], ["logout"], ["listsites"], ["get", "/views/a/b"], ["creategroup", "the-group"]]
+
+
+def real_parser() -> argparse.ArgumentParser:
+    manager = ParentParser()
+    manager.connect_commands()
+    manager.include_help()
+    return manager.get_root_parser()
+
+
+def parse(parser: argparse.ArgumentParser, args: list) -> dict:
+    values = vars(parser.parse_args(args))
+    values.pop("func")
+    return values
+
+
+class GlobalOptionsRealCommandsTest(unittest.TestCase):
+    """The same checks against the parser the CLI builds, not a single mocked command."""
+
+    parser: argparse.ArgumentParser
+
+    @classmethod
+    def setUpClass(cls):
+        cls.parser = real_parser()
+
+    def test_every_global_option_means_the_same_before_and_after_the_command(self):
+        for command in COMMANDS:
+            defaults = parse(self.parser, command)
+            for option in GLOBAL_OPTIONS:
+                with self.subTest(command=command[0], option=option):
+                    before = parse(self.parser, option + command)
+                    after = parse(self.parser, command + option)
+                    assert before == after, (before, after)
+                    assert before != defaults, "{} had no effect".format(option)
+
+    def test_option_values_before_the_command_are_converted(self):
+        values = parse(
+            self.parser, ["-l", "debug", "--language", "DE", "--country", "FR", "--query-page-size", "5", "login"]
+        )
+        assert values["logging_level"] == "DEBUG", values
+        assert values["language"] == "de", values
+        assert values["country"] == "fr", values
+        assert values["query_page_size"] == 5, values
+
+    def test_boolean_flags_default_to_false_and_are_kept_before_the_command(self):
+        flags = {
+            "--no-prompt": "no_prompt",
+            "--no-certcheck": "no_certcheck",
+            "--no-cookie": "no_cookie",
+            "--no-proxy": "no_proxy",
+            "--continue-if-exists": "continue_if_exists",
+        }
+        defaults = parse(self.parser, ["login"])
+        for flag, name in flags.items():
+            with self.subTest(flag=flag):
+                assert defaults[name] is False, defaults
+                assert parse(self.parser, [flag, "login"])[name] is True
+
+    def test_every_global_default_is_present_for_every_command(self):
+        expected = {a.dest for a in parent_parser_with_global_options()._actions if a.default is not argparse.SUPPRESS}
+        assert len(expected) >= 19, expected
+        for command in COMMANDS:
+            with self.subTest(command=command[0]):
+                missing = expected - set(parse(self.parser, command))
+                assert not missing, missing
+
+    def test_help_command_accepts_global_options_in_either_position(self):
+        defaults = parse(self.parser, ["help"])
+        assert defaults["server"] is None, defaults
+        assert defaults["logging_level"] == "INFO", defaults
+        assert parse(self.parser, ["-s", "the-server", "help"])["server"] == "the-server"
+        assert parse(self.parser, ["help", "-s", "the-server"])["server"] == "the-server"
+
+
+if __name__ == "__main__":
+    unittest.main()