From 81bbc125cdf897ada77e448115a9c72b61922cb0 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Mon, 3 Aug 2026 18:22:03 -0700 Subject: [PATCH 1/2] Fix #433: accept Classic `delete --workbook Name` syntax tabcmd Classic invokes delete as `tabcmd delete --workbook "Name"` where --workbook takes the target name as its value. tabcmd 2 shipped `tabcmd delete "Name" --workbook` (name positional, flag is a type selector). This is a breaking CLI difference for anyone running Classic scripts unchanged. Accept both forms: - Positional name stays optional (nargs="?"). - --workbook/--datasource take an optional value (nargs="?"). Bare form still means True (tabcmd 2 syntax); with a value, the value is the target name (Classic syntax). - In run_command, resolve which form supplied the name and normalize args.workbook/args.datasource to True. Emit the same "requires workbook or datasource" error when neither form supplied a name. Mutual exclusion between --workbook and --datasource still holds. Fixes #433. --- .../delete_command.py | 17 ++++++++- tabcmd/execution/global_options.py | 14 ++++++-- tests/parsers/test_parser_delete.py | 36 +++++++++++++++---- 3 files changed, 58 insertions(+), 9 deletions(-) diff --git a/tabcmd/commands/datasources_and_workbooks/delete_command.py b/tabcmd/commands/datasources_and_workbooks/delete_command.py index 71085c74..bbef034e 100644 --- a/tabcmd/commands/datasources_and_workbooks/delete_command.py +++ b/tabcmd/commands/datasources_and_workbooks/delete_command.py @@ -22,7 +22,9 @@ class DeleteCommand(DatasourcesAndWorkbooks): @staticmethod def define_args(delete_parser): group = delete_parser.add_argument_group(title=DeleteCommand.name) - group.add_argument("name", help=_("tabcmd.delete.target.name")) + # nargs="?" so `tabcmd delete --workbook "Name"` (Classic) also works, + # where the name is carried by the --workbook/--datasource value. + group.add_argument("name", nargs="?", default=None, help=_("tabcmd.delete.target.name")) set_ds_xor_wb_options(group) set_project_r_arg(group) set_parent_project_arg(group) @@ -33,7 +35,20 @@ def run_command(cls, args): logger.debug(_("tabcmd.launching")) session = Session() server = session.create_session(args, logger) + # Resolve target from either form: + # tabcmd 2: delete "Name" --workbook -> args.name="Name", args.workbook=True + # Classic: delete --workbook "Name" -> args.name=None, args.workbook="Name" content_type: str = "" + if isinstance(args.workbook, str): + if args.name is None: + args.name = args.workbook + args.workbook = True + if isinstance(args.datasource, str): + if args.name is None: + args.name = args.datasource + args.datasource = True + if args.name is None: + Errors.exit_with_error(logger, _("delete.errors.requires_workbook_datasource")) if args.workbook: content_type = "workbook" elif args.datasource: diff --git a/tabcmd/execution/global_options.py b/tabcmd/execution/global_options.py index 1a24133c..f83acd70 100644 --- a/tabcmd/execution/global_options.py +++ b/tabcmd/execution/global_options.py @@ -148,9 +148,19 @@ def set_resource_url_arg(parser): def set_ds_xor_wb_options(parser): + # Classic parity for `delete`: `tabcmd delete --workbook "Name"` treats + # --workbook's value as the target name. tabcmd 2 originally shipped as + # `tabcmd delete "Name" --workbook` (bare flag). Accept both forms by + # letting the flag take an optional value: bare -> True, with value -> + # value string. The command's run_command resolves which arg carries the + # name. See DeleteCommand. target_type_group = parser.add_mutually_exclusive_group(required=False) - target_type_group.add_argument("-d", "--datasource", action="store_true", help=_("tabcmd.options.datasource")) - target_type_group.add_argument("-w", "--workbook", action="store_true", help=_("tabcmd.options.workbook")) + target_type_group.add_argument( + "-d", "--datasource", nargs="?", const=True, default=False, help=_("tabcmd.options.datasource") + ) + target_type_group.add_argument( + "-w", "--workbook", nargs="?", const=True, default=False, help=_("tabcmd.options.workbook") + ) return parser diff --git a/tests/parsers/test_parser_delete.py b/tests/parsers/test_parser_delete.py index edf93ea3..225bd759 100644 --- a/tests/parsers/test_parser_delete.py +++ b/tests/parsers/test_parser_delete.py @@ -11,10 +11,15 @@ class DeleteParserTest(ParserTest): def setUpClass(cls): cls.parser_under_test = initialize_test_pieces(commandname, DeleteCommand) - def test_delete_parser_no_object(self): + def test_delete_parser_no_object_parses(self): + # With Classic-parity support (positional name optional so `--workbook Name` + # works), a bare `delete` now parses; the command-level check enforces that + # a name arrived via either form. mock_args = [commandname] - with self.assertRaises(SystemExit): - args = self.parser_under_test.parse_args(mock_args) + args = self.parser_under_test.parse_args(mock_args) + assert args.name is None, args + assert args.workbook is False, args + assert args.datasource is False, args def test_delete_parser(self): mock_args = [commandname, "ds", "-r", "proj"] @@ -22,7 +27,26 @@ def test_delete_parser(self): assert args.name == "ds", args assert args.project_name == "proj", args - def test_delete_parser_missing_args(self): - mock_args = [commandname, "--datasource"] + def test_delete_parser_bare_datasource_flag(self): + # tabcmd 2 form: `delete "Name" --datasource`. Bare --datasource -> True. + mock_args = [commandname, "ds", "--datasource"] + args = self.parser_under_test.parse_args(mock_args) + assert args.name == "ds", args + assert args.datasource is True, args + + def test_delete_parser_classic_workbook_form(self): + # Classic form: `delete --workbook "Name"`. The value is the target name. + mock_args = [commandname, "--workbook", "MyWorkbook"] + args = self.parser_under_test.parse_args(mock_args) + assert args.workbook == "MyWorkbook", args + + def test_delete_parser_classic_datasource_form(self): + mock_args = [commandname, "--datasource", "MyDatasource"] + args = self.parser_under_test.parse_args(mock_args) + assert args.datasource == "MyDatasource", args + + def test_delete_parser_mutually_exclusive_still_holds(self): + # --workbook and --datasource are still mutually exclusive. + mock_args = [commandname, "--workbook", "A", "--datasource", "B"] with self.assertRaises(SystemExit): - args = self.parser_under_test.parse_args(mock_args) + self.parser_under_test.parse_args(mock_args) From 1d6a7661a61cd2d47b846381cc44fa5d9a4517d5 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Mon, 10 Aug 2026 16:17:19 -0700 Subject: [PATCH 2/2] Add missing parser tests for delete syntax variations Covers gaps flagged in fresh-eyes review: - `-w` / `-d` short flags on Classic form - Ambiguous "positional Name + --workbook FlagValue" -- parser accepts both; run_command normalization discards flag value - Bare `--workbook` with no value (parses because nargs="?" const=True) - Classic form + -r project combined The run_command normalization itself (`isinstance(args.workbook, str) -> args.name = args.workbook; args.workbook = True`) is not covered here; tracked in the follow-up run_command coverage issue. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/parsers/test_parser_delete.py | 39 +++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/parsers/test_parser_delete.py b/tests/parsers/test_parser_delete.py index 225bd759..70931b7a 100644 --- a/tests/parsers/test_parser_delete.py +++ b/tests/parsers/test_parser_delete.py @@ -50,3 +50,42 @@ def test_delete_parser_mutually_exclusive_still_holds(self): mock_args = [commandname, "--workbook", "A", "--datasource", "B"] with self.assertRaises(SystemExit): self.parser_under_test.parse_args(mock_args) + + def test_delete_parser_classic_short_workbook_flag(self): + # Classic accepts `-w Name` as a shortcut for `--workbook Name`. + mock_args = [commandname, "-w", "MyWorkbook"] + args = self.parser_under_test.parse_args(mock_args) + assert args.workbook == "MyWorkbook", args + + def test_delete_parser_classic_short_datasource_flag(self): + # Classic accepts `-d Name` as a shortcut for `--datasource Name`. + mock_args = [commandname, "-d", "MyDatasource"] + args = self.parser_under_test.parse_args(mock_args) + assert args.datasource == "MyDatasource", args + + def test_delete_parser_positional_plus_flag_value_positional_wins(self): + # Ambiguous case: `delete Positional --workbook FlagValue`. Parser accepts + # both; run_command's normalization discards the flag value and keeps the + # positional. Locking the parsing side in here; run_command semantics are + # covered by the follow-up run_command coverage issue. + mock_args = [commandname, "Positional", "--workbook", "FlagValue"] + args = self.parser_under_test.parse_args(mock_args) + assert args.name == "Positional", args + assert args.workbook == "FlagValue", args + + def test_delete_parser_bare_workbook_flag_no_value(self): + # `delete --workbook` (no name from either form) parses because `nargs="?"` + # with `const=True` on the flag. args.workbook is True, args.name is None. + # Run-command then errors out on "requires workbook or datasource" -- the + # parser must not itself reject this. + mock_args = [commandname, "--workbook"] + args = self.parser_under_test.parse_args(mock_args) + assert args.name is None, args + assert args.workbook is True, args + + def test_delete_parser_classic_form_with_project(self): + # `delete --workbook Name -r proj` should accept both together. + mock_args = [commandname, "--workbook", "MyWorkbook", "-r", "proj"] + args = self.parser_under_test.parse_args(mock_args) + assert args.workbook == "MyWorkbook", args + assert args.project_name == "proj", args