Fix interactive and conditional options in click

aiida/backends/tests/cmdline/commands/test_code.py

@@ -36,23 +36,23 @@ def test_reachable(self):
 
     def test_interactive_remote(self):
         from aiida.orm import Code
-        os.environ['VISUAL'] = 'vim -cwq'
-        os.environ['EDITOR'] = 'vim -cwq'
+        os.environ['VISUAL'] = 'sleep 1; vim -cwq'
+        os.environ['EDITOR'] = 'sleep 1; vim -cwq'
         label = 'interactive_remote'
         user_input = '\n'.join([
             label, 'description', 'yes', 'simpleplugins.arithmetic.add', self.comp.name,
             '/remote/abs/path'])
         result = self.runner.invoke(setup_code, input=user_input)
-        self.assertIsNone(result.exception)
+        self.assertIsNone(result.exception, msg="There was an unexpected exception. Output: {}".format(result.output))
         self.assertIsInstance(Code.get_from_string('{}@{}'.format(label, self.comp.name)), Code)
 
     def test_interactive_upload(self):
         from aiida.orm import Code
-        os.environ['VISUAL'] = 'vim -cwq'
-        os.environ['EDITOR'] = 'vim -cwq'
+        os.environ['VISUAL'] = 'sleep 1; vim -cwq'
+        os.environ['EDITOR'] = 'sleep 1; vim -cwq'
         label = 'interactive_upload'
         user_input = '\n'.join([
-            label, 'description', 'no', 'simpleplugins.arithmetic.add', self.this_folder, self.this_file])
+            label, 'description', 'no', 'simpleplugins.arithmetic.add', self.comp.name, self.this_folder, self.this_file])
         result = self.runner.invoke(setup_code, input=user_input)
         self.assertIsNone(result.exception, result.output)
         self.assertIsInstance(Code.get_from_string('{}'.format(label)), Code)

aiida/cmdline/commands/code.py

@@ -1034,16 +1034,16 @@ def ensure_scripts(pre, post, summary):
 @click.option('--on-computer/--store-upload', is_eager=False, default=True, prompt='Installed on remote Computer?',
     cls=InteractiveOption)
 @options.INPUT_PLUGIN(prompt='Default input plugin', cls=InteractiveOption)
-@options.COMPUTER(prompt='Remote Computer', cls=InteractiveOption, required_fn=is_on_computer)
+@options.COMPUTER(prompt='Remote computer', cls=InteractiveOption, required_fn=is_on_computer, prompt_fn=is_on_computer)
 @click.option(
-    '--remote-abs-path', prompt='Remote path', required_fn=is_on_computer, type=click.Path(file_okay=True),
-    cls=InteractiveOption, help=('[if --installed]: the (full) absolute path on the remote machine'))
+    '--remote-abs-path', prompt='Remote path', required_fn=is_on_computer, prompt_fn=is_on_computer, type=click.Path(file_okay=True),
+    cls=InteractiveOption, help=('[if --on-computer]: the (full) absolute path on the remote machine'))
 @click.option('--code-folder', prompt='Folder containing the code', type=click.Path(file_okay=False, exists=True, readable=True),
-    required_fn=is_not_on_computer, cls=InteractiveOption,
-    help=('[if --upload]: folder containing the executable and all other files necessary for execution of the code'))
+    required_fn=is_not_on_computer, prompt_fn=is_not_on_computer, cls=InteractiveOption,
+    help=('[if --store-upload]: folder containing the executable and all other files necessary for execution of the code'))
 @click.option('--code-rel-path', prompt='Relative path of the executable', type=click.Path(dir_okay=False),
-    required_fn=is_not_on_computer, cls=InteractiveOption,
-    help=('[if --upload]: the relative path of the executable file inside the folder entered in the previous step or in --code-folder'))
+    required_fn=is_not_on_computer, prompt_fn=is_not_on_computer,  cls=InteractiveOption,
+    help=('[if --store-upload]: the relative path of the executable file inside the folder entered in the previous step or in --code-folder'))
 @options.PREPEND_TEXT()
 @options.APPEND_TEXT()
 @options.NON_INTERACTIVE()

aiida/cmdline/commands/node.py

@@ -223,7 +223,7 @@ def node_label(nodes, label, raw, force):
 
 @verdi_node.command('description')
 @arguments.NODES()
-@options.DESCRIPTION(help='Set DESCRIPTION as the new description for all NODES')
+@options.DESCRIPTION(help='Set DESCRIPTION as the new description for all NODES', default=None)
 @options.RAW(help='Display only descriptions, no extra information')
 @options.FORCE()
 @with_dbenv()

aiida/cmdline/params/options/__init__.py

@@ -120,7 +120,8 @@ def active_process_states():
 LABEL = OverridableOption('-L', '--label', type=click.STRING, metavar='LABEL', help='short name to be used as a label')
 
 
-DESCRIPTION = OverridableOption('-D', '--description', type=click.STRING, metavar='DESCRIPTION', help='a detailed description')
+DESCRIPTION = OverridableOption('-D', '--description', type=click.STRING, metavar='DESCRIPTION', help='a detailed description', default="", required=False)
+
 
 
 INPUT_PLUGIN = OverridableOption('-P', '--input-plugin', help='input plugin string', type=types.PluginParamType(group='calculations'))
@@ -172,3 +173,10 @@ def active_process_states():
 
 RAW = OverridableOption('-r', '--raw', 'raw', is_flag=True, default=False,
     help='display only raw query results, without any headers or footers')
+
+
+HOSTNAME = OverridableOption('-H', '--hostname', help='hostname')
+
+TRANSPORT = OverridableOption('-T', '--transport', help='transport type', type=types.PluginParamType(group='transports'), required=True)
+
+SCHEDULER = OverridableOption('-S', '--scheduler', help='scheduler type', type=types.PluginParamType(group='schedulers'), required=True)

aiida/cmdline/params/options/conditional.py

@@ -20,19 +20,33 @@ class ConditionalOption(click.Option):
         if the parameter is required to have a value
     """
 
-    def __init__(self, param_decls=None, required_fn=lambda ctx: True, **kwargs):
+    def __init__(self, param_decls=None, required_fn=None, **kwargs):
+
+        # note default behaviour for required: False
         self.required_fn = required_fn
+
+        # Required_fn overrides 'required', if defined
+        if required_fn is not None:
+            # There is a required_fn
+            self.required = False  # So it does not show up as 'required'
+
         super(ConditionalOption, self).__init__(param_decls=param_decls, **kwargs)
-        self.required = True
 
     def full_process_value(self, ctx, value):
         try:
             value = super(ConditionalOption, self).full_process_value(ctx, value)
+            if self.required_fn and self.value_is_missing(value):
+                if self.is_required(ctx):
+                    raise click.MissingParameter(ctx=ctx, param=self)
         except click.MissingParameter as err:
             if self.is_required(ctx):
-                raise err
+                raise
         return value
 
     def is_required(self, ctx):
         """runs the given check on the context to determine requiredness"""
-        return self.required_fn(ctx)
+
+        if self.required_fn:
+            return self.required_fn(ctx)
+        else:
+            return self.required

aiida/cmdline/params/options/interactive.py

@@ -44,30 +44,42 @@ def foo(label):
             click.echo('Labeling with label: {}'.format(label))
     """
 
-    def __init__(self, param_decls=None, switch=None, empty_ok=False, **kwargs):
+    def __init__(
+            self,
+            param_decls=None,
+            switch=None,  #
+            prompt_fn=None,
+            # empty_ok=False,
+            **kwargs):
         """
         :param param_decls: relayed to :class:`click.Option`
-        :param switch: sequence of parameter names
+        :param switch: sequence of parameter
+        :param prompt_fn: a callable that defines if the option should be asked for or not in interactive mode
         """
-        # intercept prompt kwarg
+
+        # intercept prompt kwarg; I need to pop it before calling super
         self._prompt = kwargs.pop('prompt', None)
-        if kwargs.get('required', None):
-            required_fn = kwargs.get('required_fn', lambda ctx: True)
-            kwargs['required_fn'] = lambda ctx: noninteractive(ctx) and required_fn(ctx)
 
-        # super
+        # call super
         super(InteractiveOption, self).__init__(param_decls=param_decls, **kwargs)
 
+        self.prompt_fn = prompt_fn
+
+        # I check that a prompt was actually defined.
+        # I do it after calling super so e.g. 'self.name' is defined
+        if not self._prompt:
+            raise TypeError(
+                "Interactive options need to have a prompt specified, but '{}' does not have a prompt defined".format(
+                    self.name))
+
         # other kwargs
         self.switch = switch
-        self.empty_ok = empty_ok
 
         # set callback
-        if self._prompt:
-            self._after_callback = self.callback
-            self.callback = self.prompt_callback
+        self._after_callback = self.callback
+        self.callback = self.prompt_callback
 
-        # set controll strings that trigger special features from the input prompt
+        # set control strings that trigger special features from the input prompt
         self._ctrl = {'?': self.ctrl_help}
 
         # set prompting type
@@ -108,13 +120,6 @@ def format_help_message(self):
         msg = click.style('\t' + msg, fg='green')
         return msg
 
-    def unacceptably_empty(self, value):
-        """check if the value is empty and should not be passed on to conversion"""
-        result = not value and not isinstance(value, bool)
-        if self.empty_ok:
-            return False
-        return result
-
     def full_process_value(self, ctx, value):
         """
         catch errors raised by ConditionalOption in order to adress them in
@@ -146,10 +151,8 @@ def simple_prompt_loop(self, ctx, param, value):
             # prompt
             value = self.prompt_func(ctx)
             if value in self._ctrl:
-                # dispatch
+                # dispatch - e.g. show help
                 self._ctrl[value]()
-            elif self.unacceptably_empty(value):
-                # repeat prompting without trying to convert
                 continue
             else:
                 # try to convert, if unsuccessful continue prompting
@@ -166,42 +169,40 @@ def after_callback(self, ctx, param, value):
     def prompt_callback(self, ctx, param, value):
         """decide wether to initiate the prompt_loop or not"""
 
-        # a value was given
+        # a value was given on the command line: then  just go with validation
         if value is not None:
             return self.after_callback(ctx, param, value)
 
-        # parameter is not reqired anyway
-        if not self.is_required(ctx):
-            return self.after_callback(ctx, param, value)
+        # The same if the user specified --non-interactive
+        if noninteractive(ctx):
+            # Check if it is required
 
-        # help option was passed on the cmdline
-        if ctx.params.get('help'):
-            return self.after_callback(ctx, param, value)
+            default = self._get_default(ctx) or self.default
 
-        # no value was given
-        try:
-            # try to convert None
-            value = self.after_callback(ctx, param, self.type.convert('', param, ctx))
-            # if conversion comes up empty, make sure empty is acceptable
-            if self.unacceptably_empty(value):
-                raise click.MissingParameter(param=param)
-
-        except (click.MissingParameter, click.BadParameter):
-            # no value was given but a value is required
-            # check for BadParameter too, because convert might not check for None specifically
-
-            # no prompting allowed
-            if noninteractive(ctx):
-                # either get a default value and return ...
-                default = self._get_default(ctx) or self.default
-                if default is not None:
-                    return self.type.convert(default, param, ctx)
-                else:
-                    # ... or raise Missing Parameter
+            if default is not None:
+                # There is a default
+                value = self.type.convert(default, param, ctx)
+            else:
+                # There is no default.
+                # If required
+                if self.is_required(ctx):
                     raise click.MissingParameter()
-            # prompting allowed
+                # In the else case: no default, not required: value is None, it's just  passed to the after_callback
+            return self.after_callback(ctx, param, value)
+
+        if self.prompt_fn is None or (self.prompt_fn is not None and self.prompt_fn(ctx)):
+            # There is no prompt_fn function, or a prompt_fn function and it says we should ask for the 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)
-        return 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
+
+        # And then we call the callback
+        return self.after_callback(ctx, param, value)
 
 
 def opt_prompter(ctx, cmd, givenkwargs, oldvalues=None):

aiida/cmdline/params/options/test_conditional.py

@@ -10,6 +10,10 @@
 class ConditionalOptionTest(unittest.TestCase):
     """Unit tests for ConditionalOption."""
 
+    @classmethod
+    def setUpClass(cls):
+        cls.runner = CliRunner()
+
     def simple_cmd(self, pname, required_fn=lambda ctx: ctx.params.get('on'), **kwargs):
         """
         returns a command with two options:
@@ -138,3 +142,63 @@ def test_ba(self):
         result_rev = runner.invoke(cmd, ['--opt-a=Bla', '--b'])
         self.assertIsNotNone(result_rev.exception)
         self.assertIn('Error: Missing option "--opt-b".', result_rev.output)
+
+    def user_callback(self, ctx, param, value):
+        if not value:
+            return -1
+        elif value != 42:
+            raise click.BadParameter('invalid', param=param)
+        else:
+            return value
+
+    def setup_flag_cond(self, **kwargs):
+        """Set up a command with a flag and a customizable option that depends on it."""
+
+        @click.command()
+        @click.option('--flag', is_flag=True)
+        @click.option('--opt-a', required_fn=lambda c: c.params.get('flag'), cls=ConditionalOption, **kwargs)
+        def cmd(flag, opt_a):
+            click.echo('{}'.format(opt_a))
+
+        return cmd
+
+    def test_default(self):
+        """Test that the default still gets passed."""
+        cmd = self.setup_flag_cond(default='default')
+        result_noflag = self.runner.invoke(cmd)
+        self.assertIsNone(result_noflag.exception)
+        self.assertEqual('default\n', result_noflag.output)
+
+        result_flag = self.runner.invoke(cmd, ['--flag'])
+        self.assertIsNone(result_flag.exception)
+        self.assertEqual('default\n', result_flag.output)
+
+    def test_callback(self):
+        """Test that the callback still gets called."""
+        cmd = self.setup_flag_cond(default=23, type=int, callback=self.user_callback)
+        result_noflag = self.runner.invoke(cmd)
+        self.assertIsNotNone(result_noflag.exception)
+
+        result_flag = self.runner.invoke(cmd, ['--flag'])
+        self.assertIsNotNone(result_flag.exception)
+
+    def test_prompt_callback(self):
+        """Test that the callback gets called on prompt results."""
+        cmd = self.setup_flag_cond(prompt='A', default=23, type=int, callback=self.user_callback)
+        result_noflag = self.runner.invoke(cmd, input='\n')
+        self.assertIsNotNone(result_noflag.exception)
+        self.assertIn('A [23]: \n', result_noflag.output)
+        self.assertIn('Invalid', result_noflag.output)
+
+        result_flag = self.runner.invoke(cmd, ['--flag'], input='\n')
+        self.assertIsNotNone(result_flag.exception)
+        self.assertIn('A [23]: \n', result_flag.output)
+        self.assertIn('Invalid', result_flag.output)
+
+    def test_required(self):
+        """Test that required_fn overrides required if it evaluates to False."""
+        cmd = self.setup_flag_cond(required=True)
+        result_noflag = self.runner.invoke(cmd)
+        self.assertIsNone(result_noflag.exception)
+        result_flag = self.runner.invoke(cmd, ['--flag'])
+        self.assertIsNotNone(result_flag.exception)