Enable mypy in CI

[alexrudd2]

Three typing errors remain in with the default mypy config. They are all in base.py and can be ignored since it will be refactored soon.

By setting py.typed in the module folder, other programs built on top of pymodbus can use its typing. This works locally on my system when installed with python3 -m pip install -e .. I'm not sure if this will happen automatically when installed from PyPI; we may need to look at the typeshed project.

Current config:

[mypy]
strict_optional = False

[alexrudd2]

@janiversen I know you did not like adding adding type stubs as development dependencies, but I don't see another way to make it work.

[janiversen]

Please have a look at the config I sent you #1360

Home Assistant uses mypy in a way where whole files are ignored, we should copy that way, otherwise e.g. base.py will continue to give problems (I will be making a lot of changes there, and do not want to fight with mypy every time).

As far as I can see, we simply need to add the files to mypy.ini (keeping it for now at the level you want, by commenting out the parts not used now). By adding the files we remove the need for stubs, something I feel rather strongly about.

The reason I am against stubs, if due to maintenance, the stub have its own version, meaning when we upgrade de main e.g. pyserial, we also need to secure that the stub is upgrade...a real PITA !!!

Mypy is something (very) nice to have but is not mandatory, so it should not cause more maintenance (as with stubs).

[janiversen]

what does the file py.typed do ? I cannot see references and it is empty.

[janiversen]

Please have the whole file ignored, since there will be a lot of changes over the next short time.

[alexrudd2]

I think it makes sense for whomever is working on a file to
(1) don't make type errors OR
(2) disable the type checking temporarily during the refactor.

This can be done by adding # type: ignore of the file, I believe.

[janiversen]

Then please do that.

Adding ignore on a line basis in a file that is being modified is just putting more work towards your fellow maintainers.

[alexrudd2]

I've left this below in the config so you can see how to ignore files. It's not needed currently.

[mypy]
# exclude = pymodbus/client/base.py

Does this work for you?

[alexrudd2]

what does the file py.typed do ? I cannot see references and it is empty.

https://peps.python.org/pep-0561/

It's supposed to be empty, indicating that the module is typed, allowing other users/applications of the module to so static analysis across modules works.

[alexrudd2]

Please have a look at the config I sent you #1360

Home Assistant uses mypy in a way where whole files are ignored, we should copy that way, otherwise e.g. base.py will continue to give problems (I will be making a lot of changes there, and do not want to fight with mypy every time).

I looked at that config. There is currently no need to ignore files since mypy reports no errors (ignoring the 3 in base.py). The HomeAssistant config is more strict. Enabling the stricter config would create hundreds of errors ( I checked). It can be done later, one rule at a time.

As far as I can see, we simply need to add the files to mypy.ini (keeping it for now at the level you want, by commenting out the parts not used now). By adding the files we remove the need for stubs, something I feel rather strongly about.

With this PR mypy runs with 0 errors against all pymodbus files. The stubs are for other libraries we use.

The reason I am against stubs, if due to maintenance, the stub have its own version, meaning when we upgrade de main e.g. pyserial, we also need to secure that the stub is upgrade...a real PITA !!!
Mypy is something (very) nice to have but is not mandatory, so it should not cause more maintenance (as with stubs).

The stub packages are automatically generated by a bot in typeshed.

These PyPI packages follow PEP 561 and are automatically released (multiple times a day, when needed) by typeshed internal machinery.

pyserial releases roughly yearly, so it's not like we need to update the stubs package very frequently. Besides, if there is an update to the stubs, it will also be an update to the pyserial function types/signatures. This means that any code using those signatures would break anyway, no? The alternative is to wait for the bug to occur at runtime!

[alexrudd2]

https://peps.python.org/pep-0561/

For instance, here is the blank file in click that allows mypy to check pymodbus' usage of that library without installing stubs.

https://github.com/pallets/click/blob/main/src/click/py.typed

[janiversen]

I did not know about py.typed, that is a good idea to add.

I disagree with the stubs, but will not block the PR on that (my opinion is no better than your opinion).

So once base.py is clear I will approve the PR, The best option is to ignore the file in the configuration, because that way we can easily see what is being ignored.

Ignoring files in the configuration, also make it more transparent to activate new checks, when activating the check, the number of ignored files increases for a short while.

[janiversen]

Let me just explain in detail why base.py as file should be ignored.

Whenever an ignore (that be flake/pylint or mypy) is used a line level, it signal that the code is correct and cannot be changed to avoid the ignore (like e.g. pylint “too many methods”). In case we do not want to refactor the code then it is better to ignore the whole file as a clear statement. Before I add an ignore I try to see if the code can be refactored.

We do not want to refactor base.py right now (because that is being done in another PR), hence ignore the file.

If we start adding ignore just to get green light from mypy, then the whole exercise is pointless, because the purpose is to get better and more stable code.

Adding an ignore in the top of the file, which should only be used when the tool do not allow an ignore list in the setup. The reason is that an ignore list is very visible, whereas an ignore in a file is just one more.

[alexrudd2]

If we start adding ignore just to get green light from mypy, then the whole exercise is pointless, because the purpose is to get better and more stable code.

After the refactor, someone will have to go through and fix the mypy errors anyways. Actually, it was just easier to fix them now. (If you refactor again, OK, it didn't take long).

[janiversen]

LGTM.