Add mypy to CI file

[domendobnikar]

• Added mypy to CI file
• Added mypy to MAKE.file, so it can be run locally with make mypy command
• Added some hints to utils and RestClient

[justinc1]

Thank you.

[PolonaM]

@domendobnikar This is not needed since None case is already handled in the from_hypercore method

[domendobnikar]

Oh I forgot to remove the same check from from_hypercore method.
Do we need to raise an exception if Role is None?

[PolonaM]

It depends on the case, we have "must_exist" parameter that handles this.

[PolonaM]

All from_hypercore methods have None check, so I would leave it there. Or is there any case where it is better to have it in other methods that call from_hypercore method?

[domendobnikar]

Get methods should handle this. Like you said - we already have must_exist implemented there, might as well have the None check, so we have everything in one place.
It makes a small difference between:
from hypercore(cls, hypercore_data: dict[Any, Any]) vs from hypercore(cls, hypercore_data: Union[dict[Any, Any], None])
And the return statement:
-> Union[Role, None] vs -> Role
We just have to pick one and stick to it. I think right now half the classes use it one way and half the other way.
At least the ones that I made. That's why I put it in here, didn't even check or think about it which is my mistake :)

[PolonaM]

Yes you are right, not all have the None check in the from_hypercore method :) But since the majority have it, I would stick to this then.

[PolonaM]

@domendobnikar why is this needed? Isn't enough just to put "other: Role"? Anyway Role class is part of User module, where I need to add the type annotation, so I am also doing this in parallel with you, thus the comments...

[domendobnikar]

This is specifically for __eq__ methods. Since __eq__ is expecting an object of any type and by hinting only Role you are trying to override that supertype so MyPy won't let you override it. Right now this works in code because you know you are only checking Role vs Role, but MyPy doesn't know that hence the NotImplemented part.

[PolonaM]

Aaa ok ok, thanks for the explanation!

[justinc1]

One note @domendobnikar . There are still errors (https://github.com/ScaleComputing/HyperCoreAnsibleCollection/actions/runs/4082190817/jobs/7036274867)

plugins/module_utils/utils.py:33: in <module>
    input: dict[Any, Any], *field_names: str, ansible_hypercore_map: dict[Any, Any]
E   TypeError: 'type' object is not subscriptable

It should be Dict from typing, not dict, for python 3.8.

[domendobnikar]

It should be Dict from typing, not dict, for python 3.8.

Oh ok will fix.

[PolonaM]

One note @domendobnikar . There are still errors (https://github.com/ScaleComputing/HyperCoreAnsibleCollection/actions/runs/4082190817/jobs/7036274867)

plugins/module_utils/utils.py:33: in <module>
    input: dict[Any, Any], *field_names: str, ansible_hypercore_map: dict[Any, Any]
E   TypeError: 'type' object is not subscriptable

It should be Dict from typing, not dict, for python 3.8.

I think here we should decide if we use "from future import annotations" or using typing. I think the recomended way is to use the first one. What do you think?

[domendobnikar]

I think here we should decide if we use "from future import annotations" or using typing. I think the recomended way is to use the first one. What do you think?

We need to use from **future** import annotations to reference to the same class within the class itself; Example would be the Get methods.
Not sure if it fixes this specific error.

[PolonaM]

I think here we should decide if we use "from future import annotations" or using typing. I think the recomended way is to use the first one. What do you think?

We need to use from **future** import annotations to reference to the same class within the class itself; Example would be the Get methods. Not sure if it fixes this specific error.

Also for the tuple vs Tuple, list vs List, dict vs Dict...