transitions icon indicating copy to clipboard operation
transitions copied to clipboard

Provide features to ease static type checking

Open aleneum opened this issue 2 months ago • 6 comments

Since transitions uses runtime decoration quite excessively static type checkers have a hard time to analyse trigger and convenience functions. This has been discussed in previous issues:

  • #654
  • #426
  • #383

Functions decorators or wrapper functions might help here:

def MyModel:

    @transition(src="A", dest="B", unless=[...])
    def my_trigger(self):
        """A transition event"""
    
    another_trigger = transition(src="B", dest="C", condition=[...])

Furthermore one could consider a new flag that attempts to create convenience functions that follow common style guides. See for instance #385 where the use of enums result in functions such as is_ERROR that most linters will complain about.

aleneum avatar Apr 25 '24 09:04 aleneum

Thanks! It'd also help if the code would be typed in general

sy-be avatar May 03 '24 08:05 sy-be

Hello @sy-bee,

Thanks! It'd also help if the code would be typed in general

currently type information are stored in stub (*.pyi) files for backward compatibility reasons. Frameworks like MyPy are able to consider stub files while type checking. Some IDEs (like PyCharm) usually also consider stub files without issues. Some other IDEs (e.g. VSCode) only consider stub files of imported functions (see picture below).

vscode_stub_example

aleneum avatar May 03 '24 08:05 aleneum

Thank you for quick responese! I see it now, in my case I am using VScode with mypy and as you mentioned - VSCode does not always consider stub files. Now the issue I'm having (apart from mentioned runtime decorated functions) is related to subclassing AsyncTransition in order to override its _change_state() method. In my override method I keep it mostly as is apart from one extra call. The method signature remains the same and no matter how I try to annotate its return type - mypy returns errors. For clarity, I've kept the method as is:

from transitions import EventData
from transitions.extensions.asyncio import AsyncTransition


class MyTransition(AsyncTransition):
    async def _change_state(self, event_data: EventData) -> None:
        if hasattr(event_data.machine, "model_graphs"):
            graph = event_data.machine.model_graphs[id(event_data.model)]
            graph.reset_styling()
            graph.set_previous_transition(self.source, self.dest)
        await event_data.machine.get_state(self.source).exit(event_data)
        event_data.machine.set_state(self.dest, event_data.model)
        event_data.update(getattr(event_data.model, event_data.machine.model_attribute))
        await event_data.machine.get_state(self.dest).enter(event_data)

with the return type set to None mypy returns:

my_transition.py:6: error: Return type "Coroutine[Any, Any, None]" of "_change_state" incompatible with return type "None" in supertype "Transition"  [override]
my_transition.py:8: error: Item "None" of "Machine | None" has no attribute "model_graphs"  [union-attr]
my_transition.py:11: error: Incompatible types in "await" (actual type "Any | None", expected type "Awaitable[Any]")  [misc]
my_transition.py:11: error: Item "None" of "Machine | None" has no attribute "get_state"  [union-attr]
my_transition.py:12: error: Item "None" of "Machine | None" has no attribute "set_state"  [union-attr]
my_transition.py:13: error: Item "None" of "Machine | None" has no attribute "model_attribute"  [union-attr]
my_transition.py:14: error: Incompatible types in "await" (actual type "Any | None", expected type "Awaitable[Any]")  [misc]
my_transition.py:14: error: Item "None" of "Machine | None" has no attribute "get_state"  [union-attr]

I've tried different return type, e.g. with Coroutine[Any, Any, None] I get

...
my_transition.py:7: error: Return type "Coroutine[Any, Any, Coroutine[Any, Any, None]]" of "_change_state" incompatible with return type "Coroutine[Any, Any, None]" in supertype "AsyncTransition"  [override]
my_transition.py:7: error: Return type "Coroutine[Any, Any, Coroutine[Any, Any, None]]" of "_change_state" incompatible with return type "None" in supertype "Transition"  [override]
...

This is apart from the rest of the typing issues with missing attributes.

Could it be that stub files are outdated? Thank you!

sy-be avatar May 03 '24 09:05 sy-be

Hi @sy-be,

Could it be that stub files are outdated? Thank you!

they are not outdated but could use improvements. As transitions was initially built rather "duck-ish" and very agnostic towards passed arguments, "post-mortem typing" isn't trivial. Furthermore, it reveals some weaknesses the inheritance approach which heavily relies on overriding has (e.g. sync to async or Liskov substitution principle violations) that are not easy to fix. That's also a good thing but requires some refactoring.

I made some minor changes in master (EventData.machine is not optional any longer, Transitions.dest might be None) and when you are okay with ignoring override issues, this should appease mypy.

from transitions.extensions.asyncio import AsyncTransition, AsyncEventData


class MyTransition(AsyncTransition):
    async def _change_state(self, event_data: AsyncEventData) -> None:   # type: ignore[override]
        assert self.dest is not None
        if hasattr(event_data.machine, "model_graphs"):  # [1]
            graph = event_data.machine.model_graphs[id(event_data.model)]
            graph.reset_styling()
            graph.set_previous_transition(self.source, self.dest)
        await event_data.machine.get_state(self.source).exit(event_data)
        event_data.machine.set_state(self.dest, event_data.model)
        event_data.update(getattr(event_data.model, event_data.machine.model_attribute))
        await event_data.machine.get_state(self.dest).enter(event_data)

However, I guess that block [1] will be moved somewhere else because the graph should be updated even if no state change happened during an internal transition. _state_change should only be called if an actual state change is happening. My first guess would be to add a graph update function to machine.before_state_change or machine.after_state_change.

aleneum avatar May 07 '24 17:05 aleneum

Excellent! Thanks for your input! I like the library a lot, with better typing it'd be even better. Any plans on releasing these changes? I'm working on adding types to a large project and ideally would love to minimise the amount of type: ignores. There aren't many related to transitions, mostly this subclass I mentioned above and the attr-defined ones - which I can workaround with a decorator theoretically.

sy-be avatar May 07 '24 20:05 sy-be

Hi @sy-be,

Any plans on releasing these changes?

end of this week. I need to check open issues and see whether there are some major issues to be tackled. features will be postponed to the next release though.

aleneum avatar May 08 '24 10:05 aleneum