Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InteractiveOption: Fix validation being skipped if ! provided #6033

Merged
merged 2 commits into from
Sep 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion aiida/cmdline/commands/cmd_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.')
Expand All @@ -114,6 +113,7 @@ def setup(

# store the updated configuration
config.store()
echo.echo_success(f'created new profile `{profile.name}`.')


@verdi.command('quicksetup')
Expand Down
4 changes: 2 additions & 2 deletions aiida/cmdline/params/options/commands/computer.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ def should_call_default_memory_per_machine(ctx): # pylint: disable=invalid-name
prompt='Default number of CPUs per machine',
cls=InteractiveOption,
prompt_fn=should_call_default_mpiprocs_per_machine,
required_fn=False,
required=False,
type=click.INT,
help='The default number of MPI processes that should be executed per machine (node), if not otherwise specified.'
'Use 0 to specify no default value.',
Expand All @@ -137,7 +137,7 @@ def should_call_default_memory_per_machine(ctx): # pylint: disable=invalid-name
prompt='Default amount of memory per machine (kB).',
cls=InteractiveOption,
prompt_fn=should_call_default_memory_per_machine,
required_fn=False,
required=False,
type=click.INT,
help='The default amount of RAM (kB) that should be allocated per machine (node), if not otherwise specified.'
)
Expand Down
4 changes: 0 additions & 4 deletions aiida/cmdline/params/options/commands/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,31 +177,27 @@ def get_quicksetup_password(ctx, param, value): # pylint: disable=unused-argume
SETUP_USER_EMAIL = options.USER_EMAIL.clone(
prompt='Email Address (for sharing data)',
default=get_config_option('autofill.user.email'),
required_fn=lambda x: get_config_option('autofill.user.email') is None,
required=True,
cls=options.interactive.InteractiveOption
)

SETUP_USER_FIRST_NAME = options.USER_FIRST_NAME.clone(
prompt='First name',
default=get_config_option('autofill.user.first_name'),
required_fn=lambda x: get_config_option('autofill.user.first_name') is None,
required=True,
cls=options.interactive.InteractiveOption
)

SETUP_USER_LAST_NAME = options.USER_LAST_NAME.clone(
prompt='Last name',
default=get_config_option('autofill.user.last_name'),
required_fn=lambda x: get_config_option('autofill.user.last_name') is None,
required=True,
cls=options.interactive.InteractiveOption
)

SETUP_USER_INSTITUTION = options.USER_INSTITUTION.clone(
prompt='Institution',
default=get_config_option('autofill.user.institution'),
required_fn=lambda x: get_config_option('autofill.user.institution') is None,
required=True,
cls=options.interactive.InteractiveOption
)
Expand Down
10 changes: 8 additions & 2 deletions aiida/cmdline/params/options/interactive.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,19 @@ def process_value(self, ctx: click.Context, value: t.Any) -> t.Any:
return self.prompt_for_value(ctx)

if value == self.CHARACTER_IGNORE_DEFAULT and source is click.core.ParameterSource.PROMPT:
return None
# This means the user does not want to set a specific value for this option, so the ``!`` character is
# translated to ``None`` and processed as normal. If the option is required, a validation error will be
# raised further down below, forcing the user to specify a valid value.
value = None

try:
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

Expand Down
11 changes: 9 additions & 2 deletions aiida/cmdline/params/options/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -285,24 +285,31 @@ 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'
)

DB_PORT = OverridableOption(
'--db-port',
required=True,
type=click.INT,
help='Database server port.',
default=DEFAULT_DBINFO['port'],
Expand Down Expand Up @@ -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(
Expand Down
20 changes: 10 additions & 10 deletions docs/source/reference/command_line.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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]
Expand All @@ -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
Expand Down
15 changes: 9 additions & 6 deletions tests/cmdline/commands/test_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '[email protected]'
Expand All @@ -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)
Expand All @@ -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(
Expand All @@ -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: [email protected]"""
email: [email protected]
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
Expand All @@ -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)
Expand Down
16 changes: 16 additions & 0 deletions tests/cmdline/params/options/test_interactive.py
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,22 @@ def test_not_required_interactive_default(run_cli_command):
assert expected in result.output


def test_interactive_ignore_default_not_required_option(run_cli_command):
"""Test that if an option is not required ``!`` is accepted and is translated to ``None``."""
cmd = create_command(required=False)
result = run_cli_command(cmd, user_input='!\n')
expected = 'Opt: !\nNone\n'
assert expected in result.output


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: Opt has to be specified\nOpt: value\nvalue\n'
assert expected in result.output


def test_get_help_message():
"""Test the :meth:`aiida.cmdline.params.options.interactive.InteractiveOption.get_help_message`."""
option = InteractiveOption('-s', type=click.STRING)
Expand Down