-
Notifications
You must be signed in to change notification settings - Fork 94
Deprecate ADDITIONAL_RETRIEVE_LIST
in settings
#1066
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
Deprecate ADDITIONAL_RETRIEVE_LIST
in settings
#1066
Conversation
The `ADDITIONAL_RETRIEVE_LIST` key in the `settings` is deprected and will be removed in a fetaure release. A warning is introduced, indicating that one should use the `CalcJob.metadata.options.additional_retrieve_list` option of `aiida-core` instead (introduced in aiidateam/aiida-core@17b7718).
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.
Changes look good to me, but just suggest to look into changing the warning class to ensure users see it
warnings.warn( | ||
'The key `ADDITIONAL_RETRIEVE_LIST` in the settings input is deprecated and will be removed in ' | ||
'the future. Use the `CalcJob.metadata.options.additional_retrieve_list` input instead.', | ||
FutureWarning |
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.
Think these warnings are not shown by default in Python. This is why we use a separate class aiida.common.warnings.AiidaDeprecationWarning
(https://github.com/aiidateam/aiida-core/blob/f43a51010b3c84b1e9526d167401fc9ac07c556a/src/aiida/common/warnings.py#L15)
I am not sure if we were already using this approach in plugins, but may be a nice way to ensure users see the warning (and that it can be turned off through verdi config warnings.showdeprecations False
).
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.
Thanks @sphuber for the review and reminder! You are right, those wouldn't be shown. I'll adapt it
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.
Just for context: I actually remember now why I used FutureWarning
instead of DeprecationWarning
, because DeprecationWarnings
are silenced per default but FutureWarnings
shouldn't be. Nonetheless, I think it makes sense to use the AiiDA specific subclass for the other reason outlined above.
@t-reents if you're ok with |
Thanks for the ping @mbercx ! Yes, I am, just need to pick it up again. I'll do it later today and let you know once it's done. |
@mbercx Please have a final look |
Fixes #939.
Deprecate the
ADDITIONAL_RETRIEVE_LIST
key in thesettings
input. Instead, one should use theCalcJob.metadata.options.additional_retrieve_list
option to specify files that should be retrieved in addition to the default ones.