pytransitions / transitions

A lightweight, object-oriented finite state machine implementation in Python with many extensions
MIT License
5.49k stars 524 forks source link

Improve Code Quality #505

Closed aleneum closed 2 years ago

aleneum commented 3 years ago

The last scrutinizer report mentions 188 style issues. The code is currently not adequately documented (see #504 or #503 ). The goal must be to significantly reduce the amount of issues (below 100) or to integrate another linting service should the reconfiguration of scrutinizer represent a notable obstacle.

rafaykh90 commented 3 years ago

Hi, is there a chance of improving the code for better support of python3 features (e.g. having typehints etc)? which could also improve IDE support as well. Thanks.

aleneum commented 3 years ago

Hello @rafaykh90,

if you are talking about library classes and methods (e.g. Machine.add_transition): absolutely! I plan to generate/write pyi files for this. This should also make static analysis with for instance mypy easier.

If you are talking about model (method) hints, e.g.

model = Model()
machine = Machine(model, states=["A", "B"], transitions=[["convert", "A", "B"]], initial="A")
model.con # <-- hint here

I am not aware of any good solution to suggest types for dynamically added methods/attributes, unfortunately (context)

ulope commented 2 years ago

Mypy supports plugins which can add typechecking for dynamic attributes. There are already plugins for e.g. django, pydantic, sqlalchemy etc.

I don't know of any IDEs / editors taking advantage of this right now though.

hhamana commented 2 years ago

We've found a way to define pyi stubs for those machines. The trick relies on inheriting from an empty class in a different file, which acts as an anchor for which we can define a pyi file with the proper types. The pyi gets detected from the matching file/module name and typecheckers consider it as overriding the whole file's implementation, which is why it needs to be on a different file. Doing it one the same file as the machine as is said on that StackOverflow thread will also override the actual machine class typing, so lose on the original implementation and relevant suggestions, which is... not quite ideal. The stub file could be auto-generated from the transitions passed, but hand-made also allows for typing explicit argument expected for the trigger, which are fetched in logic through EventData (which isn't really obvious to generate). Something like this

# custom_triggers.py
class CustomTriggers:
    pass
# custom_triggers.pyi
class CustomTriggers:
    def convert(self, temperature: int = ...) -> None: ...
# custom_machine.py
from transitions import Machine, EventData

from custom_triggers import CustomTriggers

class CustomMachine(CustomTriggers):
    states = ["A", "B"]
    transitions = {'trigger': 'convert', 'source': 'A', dest': 'B', 'after': 'post_convert' }

    def __init__(self, initial: str) -> None:
        self.machine = Machine(model=self, states=self.states, transitions=self.transitions, initial=initial, send_event=True)

    def post_convert(self, event_data: EventData) -> None:
        convert_temperature = event_data.kwargs['temperature']
        print(convert_temperature)

if __name__ == "__main__":
    machine = CustomMachine(initial="A")
    machine.convert(temperature=10)

Works neatly with VSCode suggestions and Pylance / mypy typechecking.

aleneum commented 2 years ago

Hello @hhamana,

thank you for your feedback! The next branch that will soon be merged into master and eventually become transitions 0.9. contains such stub files. The files have been automatically generated and manually adjusted. EvenData has been subclassed for the sake of accurate typing for asynchronous and nested machines.

hhamana commented 2 years ago

@aleneum thank you! I just had a brief look at the incoming stub files. that's nice and will surely prove useful for what concerns transitions direct usage. My comment was more aimed for custom triggers/usage, as you mentioned not being "aware of any good solution to suggest types for dynamically added methods/attributes". That approach was a method that worked for us, concerning those dynamic, user-defined methods, which is far out of scope of those next stubs files. Stub files work well for things the library knows it's going to dynamically add, which can fit well in the Django case too, but user-defined dynamic triggers is yet another beast that would really need some arcane black magic to type automatically. Best I can realistically think of would be to document that it's still possible to provide type hints with such a manual approach.

aleneum commented 2 years ago

Hello @hhamana,

My comment was more aimed for custom triggers/usage,

ah, I see.

Stub files work well for things the library knows it's going to dynamically add, which can fit well in the Django case too, but user-defined dynamic triggers is yet another beast that would really need some arcane black magic to type automatically.

I agree. Just FYI: In https://github.com/pytransitions/transitions/issues/383 I tinkered around with docstrings to combine documentation and definition of transitions. One approach was to define transitions within a docstring and parse the model/machine configuration from there. Furthermore, it might be possible to use MarkupMachine to generate stub files. I will check this out.

Best I can realistically think of would be to document that it's still possible to provide type hints with such a manual approach.

That's a good remark. When I add a section about the 'generic' stub files for the 0.9 release I will add some tips for user-defined transitions from this discussion.

aleneum commented 2 years ago

I just pushed 0.9 changes with pyi stub files to master. I hope this will help to prevent future confusion about what class and input should be used where.