Make load_profile importable from aiida module
#6609
Conversation
load_profile() is widely used and documented. Exposing it as API under aiida module directly can make a bunch of LSP happy.
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6609 +/- ##
==========================================
+ Coverage 77.51% 77.90% +0.40%
==========================================
Files 560 567 +7
Lines 41444 42177 +733
==========================================
+ Hits 32120 32854 +734
+ Misses 9324 9323 -1 ☔ View full report in Codecov by Sentry. |
unkcpz
force-pushed
the
enh/load-profile-api
branch
from
13b8049
to
59d7923
Compare
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
unkcpz
force-pushed
the
enh/load-profile-api
branch
from
59d7923
to
bb35d26
Compare
for more information, see https://pre-commit.ci
| @@ -35,6 +42,8 @@ | |||
| ) | |||
| __paper_short__ = 'S. P. Huber et al., Scientific Data 7, 300 (2020).' | |||
|
|
|||
| __all__ = ['load_profile', 'configure_logging', 'get_config_option', 'get_profile', 'profile_context'] | |||
There was a problem hiding this comment.
I think it makes no sense to expose configure_logging to end user. But for backward compatibility, I keep it for the moment. I prefer to remove it, agree?
For load_profile and profile_context and get_config_option, they are mentioned in the documentation so I keep those. The get_profile also seems quite handy.
There was a problem hiding this comment.
In order for this to not be a breaking change, you'll have to include get_strict_version and load_ipython_extension as well.
There was a problem hiding this comment.
What you mean? It was not there, I think by adding things no change is breaking.
The user still can do:
import aiida
aiida.get_strict_versionEven without the function in the __all__.
There was a problem hiding this comment.
Yes, but if they did from aiida import * than now suddenly get_strict_version will not be available.
There was a problem hiding this comment.
In other words, I believe when __all__ is not defined, all symbols are imported when you do star import.
There was a problem hiding this comment.
I tested, that's true, I didn't know before. Then I'll add them to __all__.
This avoid the use of file filepaths which has inconsistent return types
Out of curiosity, which LSPs specifically are you talking about? The https://docs.python.org/3/tutorial/modules.html#importing-from-a-package |
|
|
||
| @property | ||
| def daemon_pid_file(self) -> str: |
There was a problem hiding this comment.
Isn't this a breaking change if the return type is changed?
There was a problem hiding this comment.
Since it is python, I don't regard it as a "breaking change". The mypy might be unhappy if you already use types on downsteram plugins. But functionality side it won't break anything.
There was a problem hiding this comment.
Sorry, I think I am not correct, the function return type changed actually.
Then my argument would be, this is the function I think should not exposed as public API.
In the end, we need a clear thoughts and go through again things and make API exposed very clear. One typical bad impl is for scheduler plugins, the API we exposed to allow user to implement new scheduler is methods start with underscore. But that is another story, happy to discuss next week in person.
I use |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
fixes #6608
load_profile()is widely used and documented. Exposing it as API under aiida module directly can make a bunch of LSP happy.