-
Notifications
You must be signed in to change notification settings - Fork 187
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
BaseRestartWorkChain
: fix handler_overrides
ignoring enabled=False
#5598
BaseRestartWorkChain
: fix handler_overrides
ignoring enabled=False
#5598
Conversation
In commit `7b8c61d0869751455993891cfe5cf6c637593344`, the input that allows overriding handlers, `handler_overrides`, was updated to include the possibility of overriding the priority. The changes broke the original behavior though where a handler that was disabled by default could not be enabled by specifying `enabled=False` in the overrides.
6261b8b
to
7287434
Compare
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.
@sphuber thanks! Just a minor request for the clarity of the if-else statement. Otherwise, it is all good.
@@ -372,7 +372,7 @@ def get_process_handlers_by_priority(self) -> List[Tuple[int, FunctionType]]: | |||
enabled = overrides.pop('enabled', None) | |||
priority = overrides.pop('priority', None) | |||
|
|||
if enabled is False or not handler.enabled: # type: ignore[attr-defined] | |||
if enabled is False or (enabled is None and not handler.enabled): # type: ignore[attr-defined] |
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.
if enabled is False or (enabled is None and not handler.enabled): # type: ignore[attr-defined] | |
if enabled and handler.enabled: | |
handlers.append((priority or handler.priority, handler)) | |
else: | |
continue |
Is this equal? If so it is more clear. But overall, this is hard to understand in which case the override is happening.
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 it is not equivalent, because imagine that handler.enabled
is False
, meaning the handler is disabled by default, and enabled
is True
, meaning the user is overriding it to be enabled, the conditional will evaluate to False
, causing the handler to be skipped. But it should have been enabled.
What we are testing here is the if enabled
is True
in the overrides or it is not defined at all, but the default of the handler is that it is enabled, then it should be active.
I basically wrote it as the inverse, to know when to skip a handler, i.e., if the override is False
or override
is not specified and the default is False
then the handler should not be active.
Not sure how to simplify this further. Maybe assigning intermediate variables would work?
I could write it perhaps as follows:
enabled_override = overrides.pop('enabled', None)
enabled_default = handler.enabled
if enabled_override is False or (enabled_override is None and enabled_default):
continue
Does that help?
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.
Variables with the specific name are more clear to me. I thought these two enable
control two different things. It seems like override's enable control whether do override(I then realise this is meaningless).
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.
@sphuber thanks! Look it again, I think it is clear enough, maybe a comment is more helpful than changing the variable name? But overall is good to me.
In commit
7b8c61d0869751455993891cfe5cf6c637593344
, the input thatallows overriding handlers,
handler_overrides
, was updated to includethe possibility of overriding the priority. The changes broke the
original behavior though where a handler that was disabled by default
could not be enabled by specifying
enabled=False
in the overrides.