-
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
♻️ REFACTOR: EntityExtrasMixin
-> EntityExtras
#5445
Conversation
EntityExtrasMixin
-> EntityExtras
10c13b2
to
d61e61e
Compare
60c00f3
to
e83c013
Compare
e83c013
to
f3c1166
Compare
@sphuber this is now rebased |
files_original = original_calc.base.repository.list_object_names() | ||
files_cached = calc.base.repository.list_object_names() |
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.
This was apparently missed, but I don't think I saw the deprecation warning for this. Are those warnings working properly?
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.
No, the warnings are probably not "activated" for these separate system tests. Obviously, just need to set the global variable (separate PR?)
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.
Uhm, it seems like test_daemon.py
is not being run at all anymore 😅 It accidentally got removed in 5d2f153 when the RPN workchains were moved to the nightly build. I will open a PR to reinstate them
@@ -109,7 +111,20 @@ def delete(self, pk: int) -> None: | |||
self._backend.groups.delete(pk) | |||
|
|||
|
|||
class Group(entities.Entity['BackendGroup'], entities.EntityExtrasMixin, metaclass=GroupMeta): | |||
class GroupBase: | |||
"""A namespace for group related functionality, that is not directly related to its user-facing properties.""" |
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.
Still not sure about the "not directly related to its user-facing properties" because, if anything, the extras are exactly intended to be used by users. But it is just a docstring so not that important
Co-authored-by: Sebastiaan Huber <[email protected]>
@chrisjsewell approved this so good to be merged. Would be good if you can have a look at #5468 and #5457 |
I'm just out now, feel free to merge this with a suitable message. |
Part of addressing #4976 and #5465
Similar to #5442, it moves the node/group extras methods to a separate "namespace", in a back compatible manner, and is another step towards #4976 (comment): removing another 10 methods on node (i.e. #5439 + #5442 + #5445, goes from 83 -> 48)
the following deprecations are made:
e.g.
Node.get_extra
toNode.base.extras.get
To maintain parity, this PR also makes the same change to
Group
extras