-
Notifications
You must be signed in to change notification settings - Fork 139
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
Add 'handle_once' property for unregistering an EventHandler after one event #141
Conversation
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 I'm not sure about with this approach is that it mutates the event handler that's passed in. If it's reasonable that someone might want to declare and EventHandler once and then register it multiple times in different situations, then I think a different approach is needed, because this would be surprising to users:
my_event_handler = EventHandler(..., handle_once=False)
RegisterEventHandler(my_event_handler, handle_once=True)
...
RegisterEventHandler(my_event_handler) # now handle_once is True for my_event_handler
I am not 100% sure that event handlers are supposed to be reused like that, so this might be a non-issue (I'll let @wjwwood weigh in).
Perhaps what we're looking for is that subclasses on EventHandlers also have the handle_once
option. That way, rather than passing handle_once
to RegisterEventHandler, we would pass it to the event handler itself, e.g.:
# When the talker node reaches the 'active' state, log a message and start the listener node.
register_event_handler_for_talker_reaches_active_state = launch.actions.RegisterEventHandler(
launch_ros.event_handlers.OnStateTransition(
target_lifecycle_node=lifecycle_talker, goal_state='active',
entities=[
launch.actions.LogInfo(
msg="node 'talker' reached the 'active' state, launching 'listener'."),
lifecycle_listener,
],
handle_once=True
),
# handle_once=True
)
To accomplish that we would want to add kwargs
to the constructor of OnStateTransition
and then pass them through to its parent's constructor, as is done for NodeAction
.
However, this doesn't seem to be what @wjwwood had in mind when writing #111 (it mentions adding the API to RegisterEventHandler, as you've done), so it might not be the direction to go in. I am trying to get some ideas on paper to help when @wjwwood gets a chance to look at this.
I've added a few review comments; depending on the direction some may not be relevant/worth fixing.
launch/launch/event_handler.py
Outdated
if isinstance(value, bool): | ||
self.__handle_once = value | ||
else: | ||
raise TypeError( |
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.
as a general-purpose review comment, we try to avoid nesting where possible by returning early.
This could be restructured to avoid nesting by:
if not isinstance(value, bool):
raise TypeError(...)
self.__handle_once = value
@@ -32,10 +32,11 @@ class RegisterEventHandler(Action): | |||
place. | |||
""" | |||
|
|||
def __init__(self, event_handler: EventHandler, **kwargs) -> None: | |||
def __init__(self, event_handler: EventHandler, handle_once: bool = False, **kwargs) -> None: |
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.
to force that the caller specifies the handle_once
argument by name, please add *,
before it (here's an example:
def __init__(self, *, node_name: Text, **kwargs) -> None: |
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.
👍
@@ -32,10 +32,11 @@ class RegisterEventHandler(Action): | |||
place. | |||
""" | |||
|
|||
def __init__(self, event_handler: EventHandler, **kwargs) -> None: | |||
def __init__(self, event_handler: EventHandler, handle_once: bool = False, **kwargs) -> None: | |||
"""Constructor.""" |
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.
be good to add docs for handle_once
here also
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.
👍
Definitely agree that the I'll review the code now. |
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.
w.r.t.:
what I'm not sure about with this approach is that it mutates the event handler that's passed in. If it's reasonable that someone might want to declare and EventHandler once and then register it multiple times in different situations, then I think a different approach is needed, because this would be surprising to users:
I think that this might be something people want to do, but most classes at the moment aren't written to handle this reuse pattern. There's this issue #113 which touches on the problem w.r.t. Actions and I think could be extended to EventHandlers as well.
I agree that @dhood's example where they set handle_once
on the event handler to False and then set it to True when registering it might lead to confusion. And, I do think the option needs to be part of the event handler itself rather than the register event handler action instance or the context.
Therefore, maybe we should just remove the option from the RegisterEventHandler action and only allow it to be set in the EventHandler instance itself. What do you guys think?
Re: introspection, I think we should just modify the string that describes an event handler to include whether or not handle_once
is true.
@@ -32,10 +32,11 @@ class RegisterEventHandler(Action): | |||
place. | |||
""" | |||
|
|||
def __init__(self, event_handler: EventHandler, **kwargs) -> None: | |||
def __init__(self, event_handler: EventHandler, handle_once: bool = False, **kwargs) -> None: |
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.
👍
@@ -32,10 +32,11 @@ class RegisterEventHandler(Action): | |||
place. | |||
""" | |||
|
|||
def __init__(self, event_handler: EventHandler, **kwargs) -> None: | |||
def __init__(self, event_handler: EventHandler, handle_once: bool = False, **kwargs) -> None: | |||
"""Constructor.""" |
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 can roll back one commit and update as per the review comments. |
This is useful for implementing "one shot" events. The property can be set via the constructor or setter. If 'handle_once' is set to True, then the EventHandler unregisters itself from the context after the first time it is handled. All subclasses of EventHandler pass it kwargs so they can use the new property. Tests included.
07c9f77
to
dbf1ad7
Compare
I've addressed the comments. Now the launch/launch/launch/event_handler.py Line 60 in ba77b77
Rather than adding to the description of every child of EventHandler, I think it should be added to a common interface. I can look into the common interface as part of this PR, or leave it for #103. |
Common interface sounds good to me. |
A common pattern for describing event handlers has been moved to the parent class. Subclasses should implement the 'handler_description' and 'matcher_description' properties rather than implementing a describe method.
I've implemented a common interface for event handlers (which includes the launch/launch/launch/event_handlers/on_process_exit.py Lines 114 to 119 in fec5e1d
One solution that comes to mind is to add an overridable method/property to Alternatively, we could just leave in the Ideas? |
launch/launch/event_handler.py
Outdated
@@ -59,6 +62,8 @@ def __init__( | |||
@property | |||
def entities(self): | |||
"""Getter for entities.""" | |||
# if self.__entities is None: |
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'll remove this.
launch/launch/event_handler.py
Outdated
|
||
@handle_once.setter | ||
def handle_once(self, value): | ||
"""Setter for handle_once flag.""" |
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.
might be wise to remove the public setter so that users can't get into strange situations by modifying the flag. FWICT it'd be sufficient to just set it in the constructor now. If a need comes up later, we can consider re-adding it to the API (it's harder though to remove API).
Plus other minor cleanup.
I'm just curious - is there documentation for how those Travis badge comments are generated? Is there a script or is it a very manual process? And why is it repeated multiple times in a PR? I'd like to learn more about this policy... |
They're generated for each job on https://ci.ros2.org/, but the group is generated as the output of our "launcher" job: https://ci.ros2.org/job/ci_launcher/ There are some more generic notes about our pull request workflow and how CI works here: https://github.com/ros2/ros2/wiki/ROS-2-On-boarding-Guide#developer-workflow |
Add 'handle_once' property to EventHandler
This is useful for implementing "one shot" events.
The property can be set via the constructor or setter.
If 'handle_once' is set to True, then the EventHandler unregisters itself from the context after the first time it is handled.
Tests included.
Add 'handle_once' option to RegisterEventHandler
This allows the user to configure "one shot" events via launch actions.
Closes #111.
Motivated by #90, now the snippet can be modified so that if the talker cycles between "inactive" and "active" states, we avoid multiple listeners from starting:
For example, the talker can be cycled between states with
ros2 lifecycle set /talker deactivate
.Here's the diff from the original (launch introspection also added):
Introspection output:
Maybe it's worth looking into communicating the "handle once" aspect via the introspector somehow...