pytransitions / transitions

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

Use model_attribute in predicate method name #473

Closed artofhuman closed 3 years ago

artofhuman commented 3 years ago

This PR adds model_attribute to predicate method name if model_attribute != default value (state)

It needs when we are using multiple states in the model. Currently transitions generates only single method is_{state_name}. And if we have the same states in the model as in the test case. ['A', 'B'] for first state and ['A', 'B'] for the second state we can't use predicate methods because the instance already has this predicate for first state

Solutuon: separate predicate methods by name is_{method_attr}_{state_name}

coveralls commented 3 years ago

Coverage Status

Coverage increased (+0.004%) to 97.096% when pulling 2e75607053f928b52699178de10a81c83c354d62 on artofhuman:user-model-attr-for-predicate into 9b98ab26ca146b389f31fe1eef84e102c2e619e0 on pytransitions:master.

aleneum commented 3 years ago

Hi @artofhuman,

I think this change is reasonable. I fear that it will break someones code though. Some users use model_attribute just to rename the attribute and may still rely on is_<state_name>. We should provide a ... transition... period to adjust code that potentially breaks. I also changed the order of the 'if clauses'. I was once told taught it's easier to comprehend when conditions are formulated 'positively' (ìf x == 0 rather than x != 1) whenever possible. Let me know what you think about 1354a65baaf4a990fac55553e1176274972a6e97. Additionally, we do have similar 'issues' with to_<state_name> as well. Should that also be adjusted for consistency's sake?

artofhuman commented 3 years ago

Thanks, @aleneum. I think there has the same problem with auto generated transitions. to_A, to_B. I think it also should use a prefix

aleneum commented 3 years ago

I feel the same way. I will add this and hopefully publish 0.8.3 in the next days.