-
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
Make load_profile
importable from aiida module
#6609
base: main
Are you sure you want to change the base?
Changes from all commits
21811b7
f243d47
3abfe5e
bb35d26
8d157eb
33dc94c
2e2b5f4
f015ca7
40a4e4f
691f59f
a5b6f6d
2932ee3
375ba4d
ec71c9f
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 |
---|---|---|
|
@@ -156,31 +156,31 @@ | |
|
||
@property | ||
def circus_log_file(self) -> str: | ||
return self.profile.filepaths['circus']['log'] | ||
return str(self.profile.circus_log_file) | ||
|
||
@property | ||
def circus_pid_file(self) -> str: | ||
return self.profile.filepaths['circus']['pid'] | ||
return str(self.profile.circus_pid_file) | ||
|
||
@property | ||
def circus_port_file(self) -> str: | ||
return self.profile.filepaths['circus']['port'] | ||
return str(self.profile.circus_port_file) | ||
|
||
@property | ||
def circus_socket_file(self) -> str: | ||
return self.profile.filepaths['circus']['socket']['file'] | ||
return str(self.profile.circus_socket_endpoints['file']) | ||
|
||
@property | ||
def circus_socket_endpoints(self) -> dict[str, str]: | ||
return self.profile.filepaths['circus']['socket'] | ||
return {k: str(v) for k, v in self.profile.circus_socket_endpoints.items()} | ||
|
||
@property | ||
def daemon_log_file(self) -> str: | ||
return self.profile.filepaths['daemon']['log'] | ||
return str(self.profile.daemon_log_file) | ||
|
||
@property | ||
def daemon_pid_file(self) -> str: | ||
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. Isn't this a breaking change if the return type is changed? 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I think I am not correct, the function return type changed actually. 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 |
||
return self.profile.filepaths['daemon']['pid'] | ||
return str(self.profile.daemon_pid_file) | ||
|
||
def get_circus_port(self) -> int: | ||
"""Retrieve the port for the circus controller, which should be written to the circus port file. | ||
|
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.
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
andprofile_context
andget_config_option
, they are mentioned in the documentation so I keep those. Theget_profile
also seems quite handy.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.
In order for this to not be a breaking change, you'll have to include
get_strict_version
andload_ipython_extension
as well.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.
What you mean? It was not there, I think by adding things no change is breaking.
The user still can do:
Even without the function in the
__all__
.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.
Yes, but if they did
from aiida import *
than now suddenlyget_strict_version
will not be available.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested, that's true, I didn't know before. Then I'll add them to
__all__
.