ENH: correction and update of verify trigger and action methods and implementation of new tests, mostly for verify actions method.

Author: juliomachad0

rocketpy/simulation/events.py

@@ -123,8 +123,12 @@ def __verify_trigger(self, trigger):
             (at least if not declared or annotated).
         """
         # verify if the trigger function accepts only **kwargs arguments
+        # also avoids functions with no arguments, since they can't be used as triggers
         s = inspect.signature(trigger)
-        if any(p.kind != inspect.Parameter.VAR_KEYWORD for p in s.parameters.values()):
+        if (
+            any(p.kind != inspect.Parameter.VAR_KEYWORD for p in s.parameters.values())
+            or len(s.parameters) == 0
+        ):
             raise ValueError(
                 f"The Trigger function of the {self.name} event must accept only keyword arguments. def {trigger.__name__}(**kwargs) -> bool:"
             )
@@ -159,20 +163,25 @@ def __verify_action(self, action):
         """
         # verify if the action function accepts only **kwargs arguments
         s = inspect.signature(action)
-        if any(p.kind != inspect.Parameter.VAR_KEYWORD for p in s.parameters.values()):
+        if (
+            any(p.kind != inspect.Parameter.VAR_KEYWORD for p in s.parameters.values())
+            or len(s.parameters) == 0
+        ):
             raise ValueError(
-                f"The Action function of the {self.name} event must accept only keyword arguments. def {action.__name__}(**kwargs) -> None or dict:"
+                f"The Action function of the {self.name} event must accept only keyword arguments. def {action.__name__}(**kwargs) -> None | dict:"
             )
         # verify if the return type annotation is None or dict
         # Since is not possible to know for sure if the user is actually returning None or a dict,
         # we enforce None or dict annotation to motivate users to actually return None or dict.
-        return_annotation = get_type_hints(action).get("return", None)
-        if return_annotation is not None and return_annotation is not (
-            type(None) or dict
+        return_annotation = get_type_hints(action).get("return", int)
+        if (
+            (return_annotation is not type(None))
+            and (return_annotation is not dict)
+            and (return_annotation is not bool)
         ):
             raise ValueError(
                 f"The Action function of the {self.name} event must return None or a dictionary and must be annotated with '-> None' or '-> dict' for type checking.\n"
-                f"def {action.__name__}(**kwargs) -> None or dict:"
+                f"def {action.__name__}(**kwargs) -> None | dict:"
             )
         return action
 

rocketpy/simulation/flight.py

@@ -1048,10 +1048,10 @@ def __check_simulation_events(self, phase, phase_index, node_index):
 
         return False
 
-    def __handle_out_of_rail_event(self, **kwargs):
+    def __handle_out_of_rail_event(self, **kwargs) -> bool:
         """Handle the out of rail event.
 
-        Parameters
+        keyword arguments are passed by the Event class when the trigger function is called.
         ----------
         phase : FlightPhase
             The current flight phase.
@@ -1065,9 +1065,9 @@ def __handle_out_of_rail_event(self, **kwargs):
         bool
             True to indicate the simulation should break.
         """
-        phase = kwargs.get("phase")
-        phase_index = kwargs.get("phase_index")
-        node_index = kwargs.get("node_index")
+        phase = kwargs["phase"]
+        phase_index = kwargs["phase_index"]
+        node_index = kwargs["node_index"]
         # Check exactly when it went out using root finding
         # Disconsider elevation
         self.solution[-2][3] -= self.env.elevation

tests/unit/simulation/test_events.py

@@ -7,18 +7,35 @@ def test_verify_trigger_accepts_only_kwargs():
     def trigger(**kwargs) -> bool:
         return True
 
-    def action(**kwargs):
+    def action(**kwargs) -> None:
         return None
 
     event = Event(trigger=trigger, action=action, name="test")
     assert event.trigger is trigger
 
 
+def test_verify_trigger_evaluation_of_number_of_parameters():
+    def trigger(**kwargs) -> bool:
+        a = kwargs["a"]
+        b = kwargs["b"]
+        c = kwargs["c"]
+        return a + b + c == 6
+
+    def action(**kwargs) -> None:
+        return None
+
+    kwargs_test = {"a": 1, "b": 2, "c": 3}
+    assert trigger(**kwargs_test)
+
+    event = Event(trigger=trigger, action=action, name="test")
+    assert event.trigger is trigger
+
+
 def test_verify_trigger_rejects_missing_kwargs():
     def trigger(a, b) -> bool:
         return True
 
-    def action(**kwargs):
+    def action(**kwargs) -> None:
         return None
 
     with pytest.raises(
@@ -32,7 +49,21 @@ def test_verify_trigger_rejects_args_with_kwargs():
     def trigger(a, b, **kwargs) -> bool:
         return True
 
-    def action(**kwargs):
+    def action(**kwargs) -> None:
+        return None
+
+    with pytest.raises(
+        ValueError,
+        match=r"The Trigger function of the test event must accept only keyword arguments. def trigger\(\*\*kwargs\) -> bool:",
+    ):
+        Event(trigger=trigger, action=action, name="test")
+
+
+def test_verify_trigger_rejects_triggers_with_no_parameters():
+    def trigger() -> bool:
+        return True
+
+    def action(**kwargs) -> None:
         return None
 
     with pytest.raises(
@@ -46,7 +77,7 @@ def test_verify_trigger_rejects_triggers_without_bool_return_annotation():
     def trigger(**kwargs):
         return True
 
-    def action(**kwargs):
+    def action(**kwargs) -> None:
         return None
 
     with pytest.raises(
@@ -55,3 +86,80 @@ def action(**kwargs):
         + r"def trigger\(\*\*kwargs\) -> bool\:",
     ):
         Event(trigger=trigger, action=action, name="test")
+
+
+# The following tests verify if action functions were correctly implemented
+
+
+def test_verify_action_accepts_only_kwargs():
+    def trigger(**kwargs) -> bool:
+        return True
+
+    def action(**kwargs) -> None:
+        return None
+
+    event = Event(trigger=trigger, action=action, name="test")
+    assert event.action is action
+
+
+def test_verify_action_rejects_missing_kwargs():
+    def trigger(**kwargs) -> bool:
+        return True
+
+    def action(a, b) -> None:
+        return None
+
+    with pytest.raises(
+        ValueError,
+        match=r"The Action function of the test event must accept only keyword arguments. def action\(\*\*kwargs\) -> None \| dict:",
+    ):
+        Event(trigger=trigger, action=action, name="test")
+
+
+def test_verify_action_rejects_args_with_kwargs():
+    def trigger(**kwargs) -> bool:
+        return True
+
+    def action(a, b, **kwargs) -> None:
+        return None
+
+    with pytest.raises(
+        ValueError,
+        match=r"The Action function of the test event must accept only keyword arguments. def action\(\*\*kwargs\) -> None \| dict:",
+    ):
+        Event(trigger=trigger, action=action, name="test")
+
+
+def test_verify_action_accepts_dict_return_type():
+    def trigger(**kwargs) -> bool:
+        return True
+
+    def action(**kwargs) -> dict:
+        return {"key": "value"}
+
+    event = Event(trigger=trigger, action=action, name="test")
+    assert event.action is action
+
+
+def test_verify_action_accepts_none_return_type():
+    def trigger(**kwargs) -> bool:
+        return True
+
+    def action(**kwargs) -> None:
+        return None
+
+    event = Event(trigger=trigger, action=action, name="test")
+    assert event.action is action
+
+
+# this was also allowed because some actions functions already return bool, they need to be updated
+# then this test can be removed and the check for bool return type can be removed from the events.py file
+def test_verify_action_accepts_bool_return_type():
+    def trigger(**kwargs) -> bool:
+        return True
+
+    def action(**kwargs) -> bool:
+        return True
+
+    event = Event(trigger=trigger, action=action, name="test")
+    assert event.action is action