-
Notifications
You must be signed in to change notification settings - Fork 192
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
FIX: Do not evaluate callable defaults during tab-completion #6144
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
# -*- coding: utf-8 -*- | ||
########################################################################### | ||
# Copyright (c), The AiiDA team. All rights reserved. # | ||
# This file is part of the AiiDA code. # | ||
# # | ||
# The code is hosted on GitHub at https://github.com/aiidateam/aiida-core # | ||
# For further information on the license, see the LICENSE.txt file # | ||
# For further information please visit http://www.aiida.net # | ||
########################################################################### | ||
""" | ||
.. py:module::callable | ||
:synopsis: A monkey-patched subclass of click.Option that does not evaluate callable default during tab completion | ||
""" | ||
|
||
import typing as t | ||
|
||
import click | ||
|
||
__all__ = ('CallableDefaultOption',) | ||
|
||
|
||
class CallableDefaultOption(click.Option): | ||
"""A monkeypatch for click.Option that does not evaluate default callbacks during tab completion | ||
|
||
This is a temporary solution until a proper fix is implemented in click, see: | ||
https://github.com/pallets/click/issues/2614 | ||
""" | ||
|
||
def get_default(self, ctx: click.Context, call: bool = True) -> t.Optional[t.Union[t.Any, t.Callable[[], t.Any]]]: | ||
"""provides the functionality of :meth:`click.Option.get_default`, | ||
but ensures we do not evaluate callable defaults when in tab-completion context.""" | ||
if ctx.resilient_parsing: | ||
return None | ||
return super().get_default(ctx=ctx, call=call) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
|
||
from .. import types | ||
from ...utils import defaults, echo # pylint: disable=no-name-in-module | ||
from .callable import CallableDefaultOption | ||
from .config import ConfigFileOption | ||
from .multivalue import MultipleValueOption | ||
from .overridable import OverridableOption | ||
|
@@ -110,8 +111,10 @@ def set_log_level(_ctx, _param, value): | |
log.CLI_ACTIVE = True | ||
|
||
# If the value is ``None``, it means the option was not specified, but we still configure logging for the CLI | ||
# However, we skip this when we are in a tab-completion context. | ||
if value is None: | ||
configure_logging() | ||
if not _ctx.resilient_parsing: | ||
configure_logging() | ||
Comment on lines
+116
to
+117
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am a bit surprised by this change. This function There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, sorry for not being clear, I am also confused, but you can try when you remove it the test fails. But when I test the completion on the actual command line it is not called. Maybe the click function used in the test is not exactly the one that gets called?? Btw: I was testing on BASH, wonder if other shells may behave differently. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps there is some weird interaction with the test suite. Not sure if it is worth deeper investigation, since the change itself seems like an okay thing to do on its own. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Figured it out. I tested whether this was being called when actually tab-completing |
||
return None | ||
|
||
try: | ||
|
@@ -145,6 +148,7 @@ def set_log_level(_ctx, _param, value): | |
'profile', | ||
type=types.ProfileParamType(), | ||
default=defaults.get_default_profile, | ||
cls=CallableDefaultOption, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this really the only option that has an expensive callable default? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the only place where the config/profile is loaded, all the other use the InteractiveOption where this is already handled. But you are right that there likely are other expensive defaults, but I plan to look into this in a followup PR where I will also look at the timings more closely. |
||
help='Execute the command for this profile instead of the default profile.' | ||
) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
# -*- coding: utf-8 -*- | ||
########################################################################### | ||
# Copyright (c), The AiiDA team. All rights reserved. # | ||
# This file is part of the AiiDA code. # | ||
# # | ||
# The code is hosted on GitHub at https://github.com/aiidateam/aiida-core # | ||
# For further information on the license, see the LICENSE.txt file # | ||
# For further information please visit http://www.aiida.net # | ||
########################################################################### | ||
# pylint: disable=redefined-outer-name | ||
"""Tests for the :mod:`aiida.cmdline.params.options.callable` module.""" | ||
from click.shell_completion import ShellComplete | ||
import pytest | ||
|
||
from aiida.cmdline.commands.cmd_verdi import verdi | ||
|
||
|
||
def _get_completions(cli, args, incomplete): | ||
comp = ShellComplete(cli, {}, cli.name, '_CLICK_COMPLETE') | ||
return comp.get_completions(args, incomplete) | ||
|
||
|
||
@pytest.fixture | ||
def unload_config(): | ||
"""Temporarily unload the config by setting ``aiida.manage.configuration.CONFIG`` to ``None``.""" | ||
from aiida.manage import configuration | ||
|
||
config = configuration.CONFIG | ||
configuration.CONFIG = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NOTE: I have removed the try-except block, I don't think it is necessary, https://docs.pytest.org/en/latest/how-to/fixtures.html#teardown-cleanup-aka-fixture-finalization https://docs.pytest.org/en/latest/how-to/fixtures.html#safe-teardowns |
||
yield | ||
configuration.CONFIG = config | ||
|
||
|
||
@pytest.mark.usefixtures('unload_config') | ||
def test_callable_default_resilient_parsing(): | ||
"""Test that tab-completion of ``verdi`` does not evaluate defaults that load the config, which is expensive.""" | ||
from aiida.manage import configuration | ||
|
||
assert configuration.CONFIG is None | ||
completions = [c.value for c in _get_completions(verdi, [], '')] | ||
assert 'help' in completions | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed that we do not test autocompletion anywhere else in the test suite. I'll try adding more tests in a separate PR, for now I added at least this simple assert. |
||
assert configuration.CONFIG is None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated fix