Conversation
armoha
left a comment
There was a problem hiding this comment.
Thanks for the contribution and feedback!
I am not yet familiar with this library, and I inferred the type by following the function call, so the type may be incorrect.
I used a union type because I confirmed that type conversion was performed.
eudplib involves many implicit type conversions, so adding accurate type hints can be cumbersome—but it's a worthwhile effort that helps identify flaws and potential bugs.
Expressions such as
int | c.EUDVariable | c.ConstExprappeared repeatedly, which caught my eye. Therefore, it seems that this part could be separated into type variables.This may not make sense because the
EUDFuncdecorator overwrites the type entirely.Might also consider adding types to decorators, referring to PEP-612
Functions decorated with @EUDFunc are called at most once when used, and the function receives arguments that are EUDVariables type-casted to the declared parameter types. I haven't studied how to properly annotate such decorators yet, so thank you for pointing me to PEP-612!
I haven’t added type hints to EUDFunc and EUDStruct yet due to their complexity. In my opinion, restructuring the code—similar to converting a dynamic class to a static one—might help untangle and alleviate the current situation.
|
Looks good to me, thanks for your contribution! |
I am not yet familiar with this library, and I inferred the type by following the function call, so the type may be incorrect.
I used a union type because I confirmed that type conversion was performed.
Expressions such as
int | c.EUDVariable | c.ConstExprappeared repeatedly, which caught my eye. Therefore, it seems that this part could be separated into type variables.This may not make sense because the
EUDFuncdecorator overwrites the type entirely.Might also consider adding types to decorators, referring to PEP-612
If the type annotation is too wrong or different from what was intended, you may close the PR.