From c53ea20a497f66bc88f68d0603cf9a32614fc4c2 Mon Sep 17 00:00:00 2001 From: Sebastiaan Huber Date: Wed, 6 Sep 2023 14:17:45 +0200 Subject: [PATCH] CLI: Usability improvements for interactive `verdi setup` There were a number of ways that a user could break the command by providing incorrect input that was not caught by validation: * The following options are now required and can no longer incorrectly be skipped with `!`: `user_email`, `user_first_name`, `user_last_name` `user_institution`, `db_engine`, `db_backend`, `db_host`, `db_port` and the `repository_path`. * For a missing parameter in interactive mode the error now reads: Error: Institution has to be specified Instead of: Error: Missing parameter: institution which should be more intuitive * The message that a profile has successfully been created is now only displayed if the storage backend initialized successfully. Before, this was shown before storage initialization, which could then still fail, making the success message confusing. --- aiida/cmdline/commands/cmd_setup.py | 2 +- aiida/cmdline/params/options/interactive.py | 5 ++++- aiida/cmdline/params/options/main.py | 11 ++++++++-- docs/source/reference/command_line.rst | 20 +++++++++---------- tests/cmdline/commands/test_setup.py | 15 ++++++++------ .../params/options/test_interactive.py | 2 +- 6 files changed, 34 insertions(+), 21 deletions(-) diff --git a/aiida/cmdline/commands/cmd_setup.py b/aiida/cmdline/commands/cmd_setup.py index b351ba502b..3f46b83e7d 100644 --- a/aiida/cmdline/commands/cmd_setup.py +++ b/aiida/cmdline/commands/cmd_setup.py @@ -89,7 +89,6 @@ def setup( config.add_profile(profile) config.set_default_profile(profile.name) load_profile(profile.name) - echo.echo_success(f'created new profile `{profile.name}`.') # Initialise the storage echo.echo_report('initialising the profile storage.') @@ -114,6 +113,7 @@ def setup( # store the updated configuration config.store() + echo.echo_success(f'created new profile `{profile.name}`.') @verdi.command('quicksetup') diff --git a/aiida/cmdline/params/options/interactive.py b/aiida/cmdline/params/options/interactive.py index d4b5e42b23..987016d598 100644 --- a/aiida/cmdline/params/options/interactive.py +++ b/aiida/cmdline/params/options/interactive.py @@ -124,7 +124,10 @@ def process_value(self, ctx: click.Context, value: t.Any) -> t.Any: return super().process_value(ctx, value) except click.BadParameter as exception: if source is click.core.ParameterSource.PROMPT and self.is_interactive(ctx): - click.echo(f'Error: {exception}') + if isinstance(exception, click.MissingParameter): + click.echo(f'Error: {self._prompt} has to be specified') + else: + click.echo(f'Error: {exception}') return self.prompt_for_value(ctx) raise diff --git a/aiida/cmdline/params/options/main.py b/aiida/cmdline/params/options/main.py index 062c365cc7..7ac11abe5c 100644 --- a/aiida/cmdline/params/options/main.py +++ b/aiida/cmdline/params/options/main.py @@ -285,17 +285,23 @@ def set_log_level(_ctx, _param, value): DB_ENGINE = OverridableOption( '--db-engine', + required=True, help='Engine to use to connect to the database.', default='postgresql_psycopg2', type=click.Choice(['postgresql_psycopg2']) ) DB_BACKEND = OverridableOption( - '--db-backend', type=click.Choice(['core.psql_dos']), default='core.psql_dos', help='Database backend to use.' + '--db-backend', + required=True, + type=click.Choice(['core.psql_dos']), + default='core.psql_dos', + help='Database backend to use.' ) DB_HOST = OverridableOption( '--db-host', + required=True, type=types.HostnameType(), help='Database server host. Leave empty for "peer" authentication.', default='localhost' @@ -303,6 +309,7 @@ def set_log_level(_ctx, _param, value): DB_PORT = OverridableOption( '--db-port', + required=True, type=click.INT, help='Database server port.', default=DEFAULT_DBINFO['port'], @@ -371,7 +378,7 @@ def set_log_level(_ctx, _param, value): ) REPOSITORY_PATH = OverridableOption( - '--repository', type=click.Path(file_okay=False), help='Absolute path to the file repository.' + '--repository', type=click.Path(file_okay=False), required=True, help='Absolute path to the file repository.' ) PROFILE_ONLY_CONFIG = OverridableOption( diff --git a/docs/source/reference/command_line.rst b/docs/source/reference/command_line.rst index 6ab004b6b5..02d15ae214 100644 --- a/docs/source/reference/command_line.rst +++ b/docs/source/reference/command_line.rst @@ -381,11 +381,11 @@ Below is a list with all available subcommands. --last-name NONEMPTYSTRING Last name of the user. [required] --institution NONEMPTYSTRING Institution of the user. [required] --db-engine [postgresql_psycopg2] - Engine to use to connect to the database. - --db-backend [core.psql_dos] Database backend to use. + Engine to use to connect to the database. [required] + --db-backend [core.psql_dos] Database backend to use. [required] --db-host HOSTNAME Database server host. Leave empty for "peer" - authentication. - --db-port INTEGER Database server port. + authentication. [required] + --db-port INTEGER Database server port. [required] --db-name NONEMPTYSTRING Name of the database to create. --db-username NONEMPTYSTRING Name of the database user to create. --db-password TEXT Password of the database user. @@ -404,7 +404,7 @@ Below is a list with all available subcommands. --broker-port INTEGER Port for the message broker. [default: 5672] --broker-virtual-host TEXT Name of the virtual host for the message broker without leading forward slash. - --repository DIRECTORY Absolute path to the file repository. + --repository DIRECTORY Absolute path to the file repository. [required] --test-profile Designate the profile to be used for running the test suite only. --config FILEORURL Load option values from configuration file in yaml @@ -485,11 +485,11 @@ Below is a list with all available subcommands. --last-name NONEMPTYSTRING Last name of the user. [required] --institution NONEMPTYSTRING Institution of the user. [required] --db-engine [postgresql_psycopg2] - Engine to use to connect to the database. - --db-backend [core.psql_dos] Database backend to use. + Engine to use to connect to the database. [required] + --db-backend [core.psql_dos] Database backend to use. [required] --db-host HOSTNAME Database server host. Leave empty for "peer" - authentication. - --db-port INTEGER Database server port. + authentication. [required] + --db-port INTEGER Database server port. [required] --db-name NONEMPTYSTRING Name of the database to create. [required] --db-username NONEMPTYSTRING Name of the database user to create. [required] --db-password TEXT Password of the database user. [required] @@ -504,7 +504,7 @@ Below is a list with all available subcommands. --broker-port INTEGER Port for the message broker. [required] --broker-virtual-host TEXT Name of the virtual host for the message broker without leading forward slash. [required] - --repository DIRECTORY Absolute path to the file repository. + --repository DIRECTORY Absolute path to the file repository. [required] --test-profile Designate the profile to be used for running the test suite only. --config FILEORURL Load option values from configuration file in yaml diff --git a/tests/cmdline/commands/test_setup.py b/tests/cmdline/commands/test_setup.py index 0f3b33daa1..887c474b48 100644 --- a/tests/cmdline/commands/test_setup.py +++ b/tests/cmdline/commands/test_setup.py @@ -49,7 +49,7 @@ def test_help(self): self.cli_runner(cmd_setup.setup, ['--help']) self.cli_runner(cmd_setup.quicksetup, ['--help']) - def test_quicksetup(self): + def test_quicksetup(self, tmp_path): """Test `verdi quicksetup`.""" profile_name = 'testing' user_email = 'some@email.com' @@ -60,7 +60,8 @@ def test_quicksetup(self): options = [ '--non-interactive', '--profile', profile_name, '--email', user_email, '--first-name', user_first_name, '--last-name', user_last_name, '--institution', user_institution, '--db-port', self.pg_test.dsn['port'], - '--db-backend', self.storage_backend_name + '--db-backend', self.storage_backend_name, '--repository', + str(tmp_path) ] self.cli_runner(cmd_setup.quicksetup, options, use_subprocess=False) @@ -83,7 +84,7 @@ def test_quicksetup(self): backend = profile.storage_cls(profile) assert backend.get_global_variable('repository|uuid') == backend.get_repository().uuid - def test_quicksetup_from_config_file(self): + def test_quicksetup_from_config_file(self, tmp_path): """Test `verdi quicksetup` from configuration file.""" with tempfile.NamedTemporaryFile('w') as handle: handle.write( @@ -94,12 +95,13 @@ def test_quicksetup_from_config_file(self): institution: EPFL db_backend: {self.storage_backend_name} db_port: {self.pg_test.dsn['port']} -email: 123@234.de""" +email: 123@234.de +repository: {tmp_path}""" ) handle.flush() self.cli_runner(cmd_setup.quicksetup, ['--config', os.path.realpath(handle.name)], use_subprocess=False) - def test_quicksetup_autofill_on_early_exit(self): + def test_quicksetup_autofill_on_early_exit(self, tmp_path): """Test `verdi quicksetup` stores user information even if command fails.""" config = configuration.get_config() assert config.get_option('autofill.user.email', scope=None) is None @@ -117,7 +119,8 @@ def test_quicksetup_autofill_on_early_exit(self): options = [ '--non-interactive', '--profile', 'testing', '--email', user_email, '--first-name', user_first_name, '--last-name', user_last_name, '--institution', user_institution, '--db-port', - self.pg_test.dsn['port'] + 100 + self.pg_test.dsn['port'] + 100, '--repository', + str(tmp_path) ] self.cli_runner(cmd_setup.quicksetup, options, raises=True, use_subprocess=False) diff --git a/tests/cmdline/params/options/test_interactive.py b/tests/cmdline/params/options/test_interactive.py index eaa3a7167f..c78b30eca7 100644 --- a/tests/cmdline/params/options/test_interactive.py +++ b/tests/cmdline/params/options/test_interactive.py @@ -497,7 +497,7 @@ def test_interactive_ignore_default_required_option(run_cli_command): """Test that if an option is required, ``!`` is translated to ``None`` and so is not accepted.""" cmd = create_command(required=True) result = run_cli_command(cmd, user_input='!\nvalue\n') - expected = 'Opt: !\nError: Missing parameter: opt\nOpt: value\nvalue\n' + expected = 'Opt: !\nError: Opt has to be specified\nOpt: value\nvalue\n' assert expected in result.output