Config: Remove use of NO_DEFAULT for Option.default
#6145
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before, the test would just fail and say that an exception was raised but it would not display the actual exception, making it difficult to debug the problem. In the case of a non-zero exit code, the stderr is printed as well.
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`.
The previous commit fixed a bug in the evaluation of defaults for various options of the `verdi setup` command. Due to the bug, these options would set a default even if the corresponding config option was not defined. Instead of no default being defined, the empty tuple `()` would be set as string value. As soon as the bug was fixed, the `test_setup_profile_uuid` test started failing since it doesn't explicitly defined values for the options `--email`, `--first-name`, `--last-name` and `--institution`.
|
@danielhollas this reveals the real source of the test breaking in your previous PR where you made the definition of |
danielhollas
requested changes
Oct 11, 2023
| @@ -206,11 +206,15 @@ def test_setup_profile_uuid(self): | |||
|
|
|||
| profile_name = 'profile-copy' | |||
| user_email = '[email protected]' | |||
| user_first_name = 'John' | |||
There was a problem hiding this comment.
Did this test work previously because these options are not required? Or because these options were not properly validated?
There was a problem hiding this comment.
it worked properly because the type for these options is NON_EMPTY_STRING so any string goes and () is cast to a string and then accepted 😅
sphuber
force-pushed
the
fix/test-verdi-setup
branch
2 times, most recently
from
d375c2e to
60758c2
Compare
danielhollas
approved these changes
Oct 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
Option.defaultproperty would return the global constantNO_DEFAULTin case the option does not specify a default. The idea wasthat 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_optioncould also return this value. Thiswould be problematic for certain CLI command options that used the
aiida.manage.configuration.get_config_optionfunction to set thedefault. If the config option was not defined, the function would return
()which is the value of theNO_DEFAULTconstant. When the optionaccepts 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 theoptions
--email,--first-name,--last-nameand--institutionthat all define a default through
get_config_optionand therefore thedefault would be actually set to
()in case the config did not specifythese 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 betweenthis case and a default never having been defined, and so the
NO_DEFAULTglobal constant is removed and replaced by
None.