Skip to content

Commit

Permalink
CLI: Ensure deprecation warnings are printed before any prompts (aiid…
Browse files Browse the repository at this point in the history
…ateam#6433)

The `click.Command` has a `deprecated` attribute, which is a bool, that
when set to `True` results in a printed deprecation warning on usage,
and adaptions of the help page to indicate the command as deprecated.

The `click.option` prompts are however invoked before that warning is
printed. This can lead to users filling out all prompts before they are
finally greeted with the deprecation message. Therefore we customize
the `click.Command` class introducing `aiida.cmdline.groups.VerdiCommand`
to move the printing logic to before the first prompt is shown.

The `VerdiCommandGroup` sets `command_class` to this new custom class
such that all `verdi` commands automatically use it. To deprecate a
command, the `deprecated` argument now just has to be set to the desired
deprecation message.
  • Loading branch information
agoscinski authored and mikibonacci committed Sep 3, 2024
1 parent 1329aaf commit 363db4f
Show file tree
Hide file tree
Showing 6 changed files with 75 additions and 9 deletions.
4 changes: 2 additions & 2 deletions docs/source/reference/command_line.rst
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ Below is a list with all available subcommands.
list List the available codes.
relabel Relabel a code.
reveal Reveal one or more hidden codes in `verdi code list`.
setup Setup a new code.
setup (Deprecated) Setup a new code (use `verdi code create`).
show Display detailed information for a code.
test Run tests for the given code to check whether it is usable.
Expand Down Expand Up @@ -531,7 +531,7 @@ Below is a list with all available subcommands.
Usage: [OPTIONS]
Setup a new profile.
(Deprecated) Setup a new profile (use `verdi profile setup`).
This method assumes that an empty PSQL database has been created and that the database
user has been created.
Expand Down
7 changes: 3 additions & 4 deletions src/aiida/cmdline/commands/cmd_code.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from aiida.cmdline.params import arguments, options, types
from aiida.cmdline.params.options.commands import code as options_code
from aiida.cmdline.utils import echo, echo_tabulate
from aiida.cmdline.utils.decorators import deprecated_command, with_dbenv
from aiida.cmdline.utils.decorators import with_dbenv
from aiida.common import exceptions


Expand Down Expand Up @@ -89,7 +89,7 @@ def set_code_builder(ctx, param, value):
# because the ``LABEL`` option has a callback that relies on the computer being already set. Execution order is
# guaranteed only for the interactive case, however. For the non-interactive case, the callback is called explicitly
# once more in the command body to cover the case when the label is specified before the computer.
@verdi_code.command('setup')
@verdi_code.command('setup', deprecated='Please use `verdi code create` instead.')
@options_code.ON_COMPUTER()
@options_code.COMPUTER()
@options_code.LABEL()
Expand All @@ -105,9 +105,8 @@ def set_code_builder(ctx, param, value):
@options.CONFIG_FILE()
@click.pass_context
@with_dbenv()
@deprecated_command('This command will be removed soon, use `verdi code create` instead.')
def setup_code(ctx, non_interactive, **kwargs):
"""Setup a new code."""
"""Setup a new code (use `verdi code create`)."""
from aiida.orm.utils.builders.code import CodeBuilder

options_code.validate_label_uniqueness(ctx, None, kwargs['label'])
Expand Down
5 changes: 2 additions & 3 deletions src/aiida/cmdline/commands/cmd_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@
from aiida.manage.configuration import Profile, load_profile


@verdi.command('setup')
@decorators.deprecated_command('This command is deprecated, use `verdi profile setup core.psql_dos` instead.')
@verdi.command('setup', deprecated='Please use `verdi profile setup` instead.')
@options.NON_INTERACTIVE()
@options_setup.SETUP_PROFILE()
@options_setup.SETUP_USER_EMAIL()
Expand Down Expand Up @@ -68,7 +67,7 @@ def setup(
test_profile,
profile_uuid,
):
"""Setup a new profile.
"""Setup a new profile (use `verdi profile setup`).
This method assumes that an empty PSQL database has been created and that the database user has been created.
"""
Expand Down
39 changes: 39 additions & 0 deletions src/aiida/cmdline/groups/verdi.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import click

from aiida.cmdline.utils.echo import echo_deprecated
from aiida.common.exceptions import ConfigurationError
from aiida.common.extendeddicts import AttributeDict
from aiida.manage.configuration import get_config
Expand Down Expand Up @@ -79,6 +80,43 @@ def __init__(self, *args, **kwargs):
self.obj = LazyVerdiObjAttributeDict(self)


class VerdiCommand(click.Command):
"""Custom command implementation to customize the logic of printing deprecation messages.
If a command is deprecated, the :class:`click.Command` adds a deprecation marker in the short help and the full
help text, and prints a deprecation warning when the command is invoked. The problem is that the deprecation warning
is printed after the prompting for parameters, which for interactive commands mean the deprecation warning comes too
late, when the user has already provided all prompts.
Here, the :meth:`click.Command.parse_args` method is overridden, which is called before the interactive options
start to prompt, such that the deprecation warning can be printed. The :meth:`click.Command.invoke` method is also
overridden in order to skip the printing of the deprecation message handled by ``click`` as that would result in
the deprecation message being printed twice.
"""

def parse_args(self, ctx: click.Context, args: t.List[str]) -> t.List[str]:
"""Given a context and a list of arguments this creates the parser and parses the arguments.
Then context is modified as necessary.
This is automatically invoked by :meth:`click.BaseCommand.make_context`.
"""
if self.deprecated:
# We are abusing click.Command `deprecated` member variable by using a
# string instead of a bool to also use it as optional deprecated message
echo_deprecated(
self.deprecated
if isinstance(self.deprecated, str) # type: ignore[redundant-expr]
else 'This command is deprecated.'
)

return super().parse_args(ctx, args)

def invoke(self, ctx: click.Context) -> t.Any:
if self.callback is not None:
return ctx.invoke(self.callback, **ctx.params)


class VerdiCommandGroup(click.Group):
"""Subclass of :class:`click.Group` for the ``verdi`` CLI.
Expand All @@ -87,6 +125,7 @@ class VerdiCommandGroup(click.Group):
"""

context_class = VerdiContext
command_class = VerdiCommand

@staticmethod
def add_verbosity_option(cmd: click.Command) -> click.Command:
Expand Down
15 changes: 15 additions & 0 deletions tests/cmdline/commands/test_code.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,21 @@ def test_help(run_cli_command):
run_cli_command(cmd_code.setup_code, ['--help'])


def test_code_setup_deprecation(run_cli_command):
"""Checks if a deprecation warning is printed in stdout and stderr."""
# Checks if the deprecation warning is present when invoking the help page
result = run_cli_command(cmd_code.setup_code, ['--help'])
assert 'Deprecated:' in result.output
assert 'Deprecated:' in result.stderr

# Checks if the deprecation warning is present when invoking the command
# Runs setup in interactive mode and sends Ctrl+D (\x04) as input so we exit the prompts.
# This way we can check if the deprecated message was printed with the first prompt.
result = run_cli_command(cmd_code.setup_code, user_input='\x04', use_subprocess=True, raises=True)
assert 'Deprecated:' in result.output
assert 'Deprecated:' in result.stderr


@pytest.mark.usefixtures('aiida_profile_clean')
def test_code_list_no_codes_error_message(run_cli_command):
"""Test ``verdi code list`` when no codes exist."""
Expand Down
14 changes: 14 additions & 0 deletions tests/cmdline/commands/test_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,20 @@ def init_profile(self, pg_test_cluster, empty_config, run_cli_command):
self.pg_test = pg_test_cluster
self.cli_runner = run_cli_command

def test_setup_deprecation(self):
"""Checks if a deprecation warning is printed in stdout and stderr."""
# Checks if the deprecation warning is present when invoking the help page
result = self.cli_runner(cmd_setup.setup, ['--help'])
assert 'Deprecated:' in result.output
assert 'Deprecated:' in result.stderr

# Checks if the deprecation warning is present when invoking the command
# Runs setup in interactive mode and sends Ctrl+D (\x04) as input so we exit the prompts.
# This way we can check if the deprecation warning was printed with the first prompt.
result = self.cli_runner(cmd_setup.setup, user_input='\x04', use_subprocess=True, raises=True)
assert 'Deprecated:' in result.output
assert 'Deprecated:' in result.stderr

def test_help(self):
"""Check that the `--help` option is eager, is not overruled and will properly display the help message.
Expand Down

0 comments on commit 363db4f

Please sign in to comment.