Skip to content

Commit

Permalink
Config: Remove use of NO_DEFAULT for Option.default
Browse files Browse the repository at this point in the history
The `Option.default` property would return the global constant
`NO_DEFAULT` in case the option does not specify a default. The idea was
that it could be used to distinguish between an option not defining a
default and one defining the default to be `None`.

The problem is that in the various methods that return a config option
value, such as `Config.get_option` could also return this value. This
would be problematic for certain CLI command options that used the
`aiida.manage.configuration.get_config_option` function to set the
default. If the config option was not defined, the function would return
`()` which is the value of the `NO_DEFAULT` constant. When the option
accepts string values, this value would often even silently be accepted
although it almost certainly is not what the user intended.

This would actually happen for the tests of `verdi setup`, which has the
options `--email`, `--first-name`, `--last-name` and `--institution`
that all define a default through `get_config_option` and therefore the
default would be actually set to `()` in case the config did not specify
these global config options.

Since for config options there is no current use-case for actually
setting a default to `None`, there is no need to distinguish between
this case and a default never having been defined, and so the `NO_DEFAULT`
global constant is removed and replaced by `None`.
  • Loading branch information
sphuber committed Oct 11, 2023
1 parent 08cba0f commit 275718c
Show file tree
Hide file tree
Showing 3 changed files with 5 additions and 10 deletions.
3 changes: 1 addition & 2 deletions aiida/cmdline/commands/cmd_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,14 @@ def _join(val):
def verdi_config_show(ctx, option):
"""Show details of an AiiDA option for the current profile."""
from aiida.manage.configuration import Config, Profile
from aiida.manage.configuration.options import NO_DEFAULT

config: Config = ctx.obj.config
profile: Profile | None = ctx.obj.profile

dct = {
'schema': option.schema,
'values': {
'default': '<NOTSET>' if option.default is NO_DEFAULT else option.default,
'default': '<NOTSET>' if option.default is None else option.default,
'global': config.options.get(option.name, '<NOTSET>'),
}
}
Expand Down
8 changes: 3 additions & 5 deletions aiida/manage/configuration/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from aiida.common.exceptions import ConfigurationError

from . import schema as schema_module
from .options import NO_DEFAULT, Option, get_option, get_option_names, parse_option
from .options import Option, get_option, get_option_names, parse_option
from .profile import Profile

__all__ = ('Config', 'config_schema', 'ConfigValidationError')
Expand Down Expand Up @@ -407,7 +407,7 @@ def set_option(self, option_name, option_value, scope=None, override=True):

if parsed_value is not None:
value = parsed_value
elif option.default is not NO_DEFAULT:
elif option.default is not None:
value = option.default
else:
return
Expand Down Expand Up @@ -442,9 +442,7 @@ def get_option(self, option_name, scope=None, default=True):
:return: the option value or None if not set for the given scope
"""
option = get_option(option_name)

# Default value is `None` unless `default=True` and the `option.default` is not `NO_DEFAULT`
default_value = option.default if default and option.default is not NO_DEFAULT else None
default_value = option.default if default else None

if scope is not None:
value = self.get_profile(scope).get_option(option.name, default_value)
Expand Down
4 changes: 1 addition & 3 deletions aiida/manage/configuration/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@

__all__ = ('get_option', 'get_option_names', 'parse_option', 'Option')

NO_DEFAULT = ()


class Option:
"""Represent a configuration option schema."""
Expand All @@ -41,7 +39,7 @@ def valid_type(self) -> Any:

@property
def default(self) -> Any:
return self._schema.get('default', NO_DEFAULT)
return self._schema.get('default', None)

@property
def description(self) -> str:
Expand Down

0 comments on commit 275718c

Please sign in to comment.