From ac3a8d8bf23c0a7900befe47db0a632c911ac7c5 Mon Sep 17 00:00:00 2001 From: Bill Flynn Date: Tue, 28 Nov 2017 17:32:13 -0500 Subject: [PATCH 1/5] Print help menu when no subcommands applied Made subparsers not required in the main parser, but if no commands are parsed, then the parser prints its help. When a command is present, the normal functionality preserved. Removed the single unittest that checks for this exact behavior. --- pubs/pubs_cmd.py | 9 ++++++--- tests/test_usecase.py | 7 ------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/pubs/pubs_cmd.py b/pubs/pubs_cmd.py index da8a43e..e849f69 100644 --- a/pubs/pubs_cmd.py +++ b/pubs/pubs_cmd.py @@ -69,7 +69,7 @@ def execute(raw_args=sys.argv): prog="pubs", add_help=True) parser.add_argument('--version', action='version', version=__version__) subparsers = parser.add_subparsers(title="valid commands", dest="command") - subparsers.required = True + #subparsers.required = True # Populate the parser with core commands for cmd_name, cmd_mod in CORE_CMDS.items(): @@ -85,8 +85,11 @@ def execute(raw_args=sys.argv): autocomplete(parser) # Parse and run appropriate command args = parser.parse_args(remaining_args) - args.prog = "pubs" # FIXME? - args.func(conf, args) + if not args.command: + parser.print_help() + else: + args.prog = "pubs" # FIXME? + args.func(conf, args) except Exception as e: if not uis.get_ui().handle_exception(e): diff --git a/tests/test_usecase.py b/tests/test_usecase.py index 506ed03..ce870cf 100644 --- a/tests/test_usecase.py +++ b/tests/test_usecase.py @@ -187,13 +187,6 @@ class URLContentTestCase(DataCommandTestCase): # Actual tests -class TestAlone(CommandTestCase): - - def test_alone_misses_command(self): - with self.assertRaises(FakeSystemExit): - self.execute_cmds(['pubs']) - - class TestInit(CommandTestCase): def test_init(self): From 66c90c5d436a85a72a6140287f012461b255e43f Mon Sep 17 00:00:00 2001 From: Bill Flynn Date: Thu, 30 Nov 2017 14:48:50 -0500 Subject: [PATCH 2/5] Added unittest to cover new behavior `pubs` Unittest just checks that both `pubs` and `pubs --help` raise a `SystemExit` exception with error code 0. Due to how argparse handles the `--help` keyword, this is the best way I could think to provide test coverage without heavily modifying the parser structure or the unittest infrastructure. To ensure the `pubs` matches the behavior of `pubs --help`, it now raises the same `SystemExit(0)` exception via `sys.exit(0)`. And in order to catch it in the unittest, I had to modify the `FakeSystemExit` behavior slightly. --- pubs/pubs_cmd.py | 10 +++++++--- tests/test_usecase.py | 26 +++++++++++++++++++++++--- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/pubs/pubs_cmd.py b/pubs/pubs_cmd.py index e849f69..e9c8b46 100644 --- a/pubs/pubs_cmd.py +++ b/pubs/pubs_cmd.py @@ -83,13 +83,17 @@ def execute(raw_args=sys.argv): # Eventually autocomplete autocomplete(parser) + # Parse and run appropriate command + # if no command, print help and exit peacefully (as '--help' does) args = parser.parse_args(remaining_args) + if not args.command: parser.print_help() - else: - args.prog = "pubs" # FIXME? - args.func(conf, args) + sys.exit(0) + + args.prog = "pubs" # FIXME? + args.func(conf, args) except Exception as e: if not uis.get_ui().handle_exception(e): diff --git a/tests/test_usecase.py b/tests/test_usecase.py index ce870cf..7ce85fc 100644 --- a/tests/test_usecase.py +++ b/tests/test_usecase.py @@ -29,7 +29,7 @@ PRINT_OUTPUT = False CAPTURE_OUTPUT = True -class FakeSystemExit(Exception): +class FakeSystemExit(SystemExit): """\ SystemExit exceptions are replaced by FakeSystemExit in the execute_cmds() function, so they can be catched by ExpectedFailure tests in Python 2.x. @@ -131,9 +131,9 @@ class CommandTestCase(fake_env.TestFakeFs): exc_class, exc, tb = sys.exc_info() if sys.version_info.major == 2: # using six to avoid a SyntaxError in Python 3.x - six.reraise(FakeSystemExit, exc, tb) + six.reraise(FakeSystemExit, *exc.args, exc_traceback=tb) else: - raise FakeSystemExit(exc).with_traceback(tb) + raise FakeSystemExit(*exc.args).with_traceback(tb) def tearDown(self): pass @@ -187,6 +187,26 @@ class URLContentTestCase(DataCommandTestCase): # Actual tests +class TestAlone(CommandTestCase): + + def test_alone_prints_help(self): + # capturing the output of `pubs --help` is difficult because argparse + # raises as SystemExit(0) after calling `print_help`, and this gets + # caught so no output is captured. so comparing outputs of `pubs` and + # `pubs --help` isn't too easy unless substantially reorganization of + # the parser and testing context is made. instead, the exit codes of + # the two usecases are compared. + self.execute_cmds(['pubs init']) + + with self.assertRaises(FakeSystemExit) as cm1: + self.execute_cmds(['pubs']) + + with self.assertRaises(FakeSystemExit) as cm2: + self.execute_cmds(['pubs --help']) + + self.assertEqual(cm1.exception.code, cm2.exception.code, 0) + + class TestInit(CommandTestCase): def test_init(self): From 0c7ba85af97ce666a07772cfa973be2d637511f5 Mon Sep 17 00:00:00 2001 From: Bill Flynn Date: Fri, 8 Dec 2017 17:49:19 -0500 Subject: [PATCH 3/5] Revisions subject to comments on PR #100 Additionally, reverted FakeSystemExit subclassing Exception, but added an explicit __init__ so that we can emulate the SystemExit.code functionality without having to change the superclass. --- pubs/pubs_cmd.py | 9 ++++----- tests/test_usecase.py | 25 +++++++++++++++++++------ 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/pubs/pubs_cmd.py b/pubs/pubs_cmd.py index e9c8b46..2edfd65 100644 --- a/pubs/pubs_cmd.py +++ b/pubs/pubs_cmd.py @@ -69,7 +69,6 @@ def execute(raw_args=sys.argv): prog="pubs", add_help=True) parser.add_argument('--version', action='version', version=__version__) subparsers = parser.add_subparsers(title="valid commands", dest="command") - #subparsers.required = True # Populate the parser with core commands for cmd_name, cmd_mod in CORE_CMDS.items(): @@ -86,11 +85,11 @@ def execute(raw_args=sys.argv): # Parse and run appropriate command # if no command, print help and exit peacefully (as '--help' does) - args = parser.parse_args(remaining_args) - + args = parser.parse_args(remaining_args) if not args.command: - parser.print_help() - sys.exit(0) + ui.error("Too few arguments!\n") + parser.print_help(file=sys.stderr) + sys.exit(2) args.prog = "pubs" # FIXME? args.func(conf, args) diff --git a/tests/test_usecase.py b/tests/test_usecase.py index 7ce85fc..41b652a 100644 --- a/tests/test_usecase.py +++ b/tests/test_usecase.py @@ -29,14 +29,21 @@ PRINT_OUTPUT = False CAPTURE_OUTPUT = True -class FakeSystemExit(SystemExit): +class FakeSystemExit(Exception): """\ SystemExit exceptions are replaced by FakeSystemExit in the execute_cmds() function, so they can be catched by ExpectedFailure tests in Python 2.x. If a code is expected to raise SystemExit, catch FakeSystemExit instead. + + Added explicit __init__ so SystemExit.code functionality could be emulated. + Taking form from https://stackoverflow.com/a/26938914/1634191 """ - pass + def __init__(self, message, code=None, *args): + self.message = message + self.code = code + + super(FakeSystemExit, self).__init__(message, *args) # code for fake fs @@ -131,7 +138,7 @@ class CommandTestCase(fake_env.TestFakeFs): exc_class, exc, tb = sys.exc_info() if sys.version_info.major == 2: # using six to avoid a SyntaxError in Python 3.x - six.reraise(FakeSystemExit, *exc.args, exc_traceback=tb) + six.reraise(FakeSystemExit, FakeSystemExit(*exc.args), tb) else: raise FakeSystemExit(*exc.args).with_traceback(tb) @@ -189,6 +196,13 @@ class URLContentTestCase(DataCommandTestCase): class TestAlone(CommandTestCase): + def test_alone_misses_command(self): + with self.assertRaises(FakeSystemExit) as cm: + self.execute_cmds(['pubs']) + self.assertEqual(cm.exception.code, 2) + + + @unittest.skipIf(sys.version_info.major == 2, "not supported for Python2") def test_alone_prints_help(self): # capturing the output of `pubs --help` is difficult because argparse # raises as SystemExit(0) after calling `print_help`, and this gets @@ -196,15 +210,14 @@ class TestAlone(CommandTestCase): # `pubs --help` isn't too easy unless substantially reorganization of # the parser and testing context is made. instead, the exit codes of # the two usecases are compared. - self.execute_cmds(['pubs init']) with self.assertRaises(FakeSystemExit) as cm1: self.execute_cmds(['pubs']) with self.assertRaises(FakeSystemExit) as cm2: - self.execute_cmds(['pubs --help']) + self.execute_cmds(['pubs', '--help']) - self.assertEqual(cm1.exception.code, cm2.exception.code, 0) + self.assertEqual(cm1.exception.code, cm2.exception.code, 2) class TestInit(CommandTestCase): From 37f076049b6502ec03d868cd6da068904a3be39a Mon Sep 17 00:00:00 2001 From: Bill Flynn Date: Fri, 8 Dec 2017 17:51:06 -0500 Subject: [PATCH 4/5] Many tests on MacOS 10.12.06 were failing due to fakefs path issues. Not sure if this is needed. Will see in the CI builds. --- tests/fake_env.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/fake_env.py b/tests/fake_env.py index 00ad0da..f239018 100644 --- a/tests/fake_env.py +++ b/tests/fake_env.py @@ -90,7 +90,7 @@ class FakeInput(): class TestFakeFs(fake_filesystem_unittest.TestCase): def setUp(self): - self.rootpath = os.path.dirname(__file__) + self.rootpath = os.path.abspath(os.path.dirname(__file__)) self.setUpPyfakefs() self.fs.CreateDirectory(self.rootpath) os.chdir(self.rootpath) From b7d135f1c87cf542bdc1e491351daf686bb69ffa Mon Sep 17 00:00:00 2001 From: Olivier Mangin Date: Mon, 11 Dec 2017 14:53:56 -0500 Subject: [PATCH 5/5] Adds wflynny to contributors. --- readme.md | 1 + 1 file changed, 1 insertion(+) diff --git a/readme.md b/readme.md index 1a060e2..21767db 100644 --- a/readme.md +++ b/readme.md @@ -125,3 +125,4 @@ You can access the self-documented configuration by using `pubs conf`, and all t - [Olivier Mangin](http://olivier.mangin.com) - Jonathan Grizou - Arnold Sykosch +- [Bill Flynn](https://github.com/wflynny)