From 36432986b74e9e2a2a2f362a02a49904c758d45f Mon Sep 17 00:00:00 2001 From: janssenhenning Date: Wed, 11 Aug 2021 13:34:11 +0200 Subject: [PATCH 1/2] FIX: ``InteractiveOption`` Remove double call of normal ``callback`` In the ``InteractiveOption`` providing a normal callback is wrapped in the method ``after_callback`` while the actual callback is replaced by ``prompt_callback``. Now the following situation occurs when the parameter should be prompted for: 1. ``prompt_loop`` is entered via ``prompt_callback`` and the value is converted in ``safely_convert`` 2. ``safely_convert`` also includes a call to ``prompt_callback``, where a call to ``after_callback`` is performed 3. After a value succeeds the code goes back into the place, where the ``prompt_loop`` was entered in ``prompt_callback`` and continues with an additional call to ``after_callback`` which already executed before The fix here is to directly return after the prompt_loop finishes and handle the case, where the value should not be prompted separately --- aiida/cmdline/params/options/interactive.py | 10 +++---- .../params/options/test_interactive.py | 29 +++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/aiida/cmdline/params/options/interactive.py b/aiida/cmdline/params/options/interactive.py index beee3a92c2..dfb3a69b3e 100644 --- a/aiida/cmdline/params/options/interactive.py +++ b/aiida/cmdline/params/options/interactive.py @@ -271,11 +271,11 @@ def prompt_callback(self, ctx, param, value): # If we are here, we are in interactive mode and the parameter is not specified # We enter the prompt loop - value = self.prompt_loop(ctx, param, value) - else: - # There is a prompt_fn function and returns False (i.e. should not ask for this value - # We then set the value to None - value = None + return self.prompt_loop(ctx, param, value) + + # There is a prompt_fn function and returns False (i.e. should not ask for this value + # We then set the value to None + value = None # And then we call the callback return self.after_callback(ctx, param, value) diff --git a/tests/cmdline/params/options/test_interactive.py b/tests/cmdline/params/options/test_interactive.py index 6b36db8f0c..a17785d61c 100644 --- a/tests/cmdline/params/options/test_interactive.py +++ b/tests/cmdline/params/options/test_interactive.py @@ -47,6 +47,21 @@ def validate_positive_number(ctx, param, value): # pylint: disable=unused-argum raise BadParameter(f'{value} is not a valid positive number') +def validate_positive_number_with_echo(ctx, param, value): # pylint: disable=unused-argument + """Validate that the number passed to this parameter is a positive number. + Also echos a message to the terminal + + :param ctx: the `click.Context` + :param param: the parameter + :param value: the value passed for the parameter + :raises `click.BadParameter`: if the value is not a positive number + """ + click.echo('Validating Number') + if not isinstance(value, (int, float)) or value < 0: + from click import BadParameter + raise BadParameter(f'{value} is not a valid positive number') + + class InteractiveOptionTest(unittest.TestCase): """Unit tests for InteractiveOption.""" @@ -95,6 +110,20 @@ def test_callback_prompt_twice(self): self.assertIn(expected_3, lines[9]) self.assertIn(expected_4, lines[12]) + def test_callback_prompt_only_once(self): + """ + scenario: using InteractiveOption with type=float and callback that echos an additional message + behaviour: the callback should be called at most once per prompt + """ + cmd = self.simple_command(type=float, callback=validate_positive_number_with_echo) + runner = CliRunner() + result = runner.invoke(cmd, [], input='string\n-1\n-1\n1\n') + self.assertIsNone(result.exception) + lines = result.output.split('\n') + #The callback should be called once per prompt + #where type conversion was successful + self.assertEqual(lines.count('Validating Number'), 3) + def test_prompt_str(self): """ scenario: using InteractiveOption with type=str From 64be0f2ac68b088148bcfb96912416ecec297199 Mon Sep 17 00:00:00 2001 From: janssenhenning Date: Thu, 12 Aug 2021 10:04:14 +0200 Subject: [PATCH 2/2] Include value in echo for test to make it more robust --- tests/cmdline/params/options/test_interactive.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/tests/cmdline/params/options/test_interactive.py b/tests/cmdline/params/options/test_interactive.py index a17785d61c..0b436e0cad 100644 --- a/tests/cmdline/params/options/test_interactive.py +++ b/tests/cmdline/params/options/test_interactive.py @@ -56,7 +56,7 @@ def validate_positive_number_with_echo(ctx, param, value): # pylint: disable=un :param value: the value passed for the parameter :raises `click.BadParameter`: if the value is not a positive number """ - click.echo('Validating Number') + click.echo(f'Validating {value}') if not isinstance(value, (int, float)) or value < 0: from click import BadParameter raise BadParameter(f'{value} is not a valid positive number') @@ -117,12 +117,21 @@ def test_callback_prompt_only_once(self): """ cmd = self.simple_command(type=float, callback=validate_positive_number_with_echo) runner = CliRunner() - result = runner.invoke(cmd, [], input='string\n-1\n-1\n1\n') + result = runner.invoke(cmd, [], input='string\n-1\n1\n') self.assertIsNone(result.exception) + expected_1 = 'Error: string is not a valid floating point value' + expected_2 = 'Validating -1.0' + expected_3 = 'Error: -1.0 is not a valid positive number' + expected_4 = 'Validating 1.0' + expected_5 = '1.0' lines = result.output.split('\n') #The callback should be called once per prompt #where type conversion was successful - self.assertEqual(lines.count('Validating Number'), 3) + self.assertEqual(expected_1, lines[3]) + self.assertEqual(expected_2, lines[6]) + self.assertEqual(expected_3, lines[7]) + self.assertEqual(expected_4, lines[10]) + self.assertEqual(expected_5, lines[11]) def test_prompt_str(self): """