eliminates implicit Optional #1817

MAKOMO · 2023-10-14T16:12:50Z

PEP 484 prohibits implicit Optional for good reasons. See for Any advantages of enabled mypy `strict_optional? some arguments.

Current mypy configuration ignores implicit Optional

[tool.mypy]
strict_optional = false

with strict_optional set mypy raises 87 errors in 13 files. This commit resolves all of them.

While this is mostly about adding/changing type annotations I had to apply few code changes too as well as one (minor) API change (see the marks below). Here are non-trivial issues I encountered. Maybe you find better resolutions than the ones proposed by this PR for those.

Subclass should follow the type declarations of its superclass

base.py:ModbusBaseClient
  self.last_frame_end: float = 0

serial.py:ModbusSerialClient(ModbusBaseClient)
  self.last_frame_end = None

resolved by widening

self.last_frame_end: float | None = 0

simple type violation

base.py:ModbusBaseClient(ModbusClientMixin, ModbusProtocol)
   def callback_disconnected(self, _reason: Exception) -> None

transport.py:ModbusProtocol(asyncio.BaseProtocol):
  def callback_disconnected(self, exc: Exception) -> None
  ..
  def transport_close(self, intern: bool = False, reconnect: bool = False) -> None:
    ..
      value.callback_disconnected(None)

resolved by widening

  def callback_disconnected(self, _reason: Exception | None)

Maybe frame should not be an optional argument but mandatory? [API CHANGE]

base.py:ModbusBaseClient(ModbusClientMixin, ModbusProtocol)

    def __init__(  # pylint: disable=too-many-arguments
           self, framer: type[ModbusFramer] | None = None,

        self.framer = framer(ClientDecoder(), self)

resolved by a cast

        self.framer = cast(type[ModbusFramer], framer)(ClientDecoder(), self)

which make pylint to fail under Python 3.8:

pymodbus/client/base.py:121:27: E1136: Value 'type' is unsubscriptable (unsubscriptable-object)

so I removed the cast and made the framer argument mandatory

    def __init__(  # pylint: disable=too-many-arguments
           self, framer: type[ModbusFramer],

This is an API change!

self.last_frame_end can be None?

base.py:ModbusBaseClient(ModbusClientMixin, ModbusProtocol)

    def __init__(  # pylint: disable=too-many-arguments
       ...
       self.last_frame_end: float = 0
       ...

    def idle_time(self) -> float:
        ...
        if self.last_frame_end is None or self.silent_interval is None:
            return 0

resolved by extending the type declaration of `self.last_frame_end

    def __init__(  # pylint: disable=too-many-arguments
       ...
       self.last_frame_end: float | None = 0
       ...

Module missing

server/simulator/http_server.py

try:
    from aiohttp import web
except ImportError:
    web = None

mypy reports

pymodbus/server/simulator/http_server.py:14: error: Incompatible types in assignment (expression has type "None", variable has type Module)  [assignment]

resolved by using a cast

try:
    from aiohttp import web
except ImportError:
    web = cast(Any, None)

see python/mypy#10512

Type violation: None is not a dict

mypy reports

pymodbus/server/simulator/http_server.py:159: error: Argument 2 to "ModbusSimulatorContext" has incompatible type "Any | None"; expected "dict[str, Callable[..., Any]]"  [arg-type]

We have

        self.datastore_context = ModbusSimulatorContext(
            device, custom_actions_dict or None )

but

ModbusSimulatorContext:
    def __init__(self, config: Dict[str, Any], custom_actions: Dict[str, Callable]
    ) -> None:


resolved by replacing the None by an empty dict

```python
        self.datastore_context = ModbusSimulatorContext(
            device, custom_actions_dict or {}
        )

ModbusProtocol.transport not a tuple [CODE CHANGE]

transport/transport.py:ModbusProtocol

def __init__(
        self,
        params: CommParams,
        is_server: bool,
    ) -> None:
...
self.transport: asyncio.BaseTransport = None


async def transport_listen(self) -> bool:

            self.transport = await self.call_create()
            if isinstance(self.transport, tuple):
                self.transport = self.transport[0]

How can self.transport is of type asyncio.BaseTransport and thus cannot hold a tuple.

            transport = await self.call_create()
            if isinstance(transport, tuple):
                self.transport = transport[0]
            else:
                self.transport = transport

baudrate in ModbusSerialClient can be None

client/serial.py

class ModbusSerialClient(ModbusBaseClient):

    def __init__(
        self,
        ...
        baudrate: int = 19200,
        ...


        ModbusBaseClient.__init__(
            self,
            ...
            baudrate=baudrate,
            ...
        )


        self._t0 = float(1 + bytesize + stopbits) / self.comm_params.baudrate

but from transport/transport.py we see that self.comm_params.baudrate could be None:

@dataclasses.dataclass
class CommParams:
    ...
    baudrate: int | None = None
    ...

resolved by adding an assertion

        assert isinstance(self.comm_params.baudrate, int)
        self._t0 = float(1 + bytesize + stopbits) / self.comm_params.baudrate

Return type declaration of init_setup_serial wrong [CODE CHANGE]

transport/transport.py

class ModbusProtocol(asyncio.BaseProtocol):

    def __init__(..) -> None:
        ...
        if self.comm_params.comm_type == CommType.SERIAL:
            host, port = self.init_setup_serial(host, port)
            if not host and not port:
                return
        ...
    def init_setup_serial(self, host: str, _port: int) -> tuple[str, int]:
        ...
        return None, None

The return type of init_setup_serial declaration does not match its implementation.

Resolved by extending this type declaration and taking care that in the assignment of host and port in __init__ does not violate its declaration:

    def __init__(..) -> None:
        ...
      if self.comm_params.comm_type == CommType.SERIAL:
            host_serial, port_serial = self.init_setup_serial(host, port)
            if host_serial is None or port_serial is None:
                return
            host, port = host_serial, port_serial
        ...
    def init_setup_serial(
        self, host: str, _port: int) -> tuple[str, int] | tuple[None, None]:
        ...
        return None, None

self.loop can be None

transport/transport.py:ModbusProtocol

class ModbusProtocol(asyncio.BaseProtocol):
    def __init__(..) -> None:
        ..
        self.loop: asyncio.AbstractEventLoop | None = None
        ..


    def init_setup_connect_listen(self, host: str, port: int) -> None:
        """Handle connect/listen handler."""
        if self.comm_params.comm_type == CommType.UDP:
            if self.is_server:
                self.call_create = lambda: self.loop.create_datagram_endpoint(
                    self.handle_new_connection,
                    local_addr=(host, port),
                )

resolved by adding a cast

    def init_setup_connect_listen(self, host: str, port: int) -> None:
        """Handle connect/listen handler."""
        if self.comm_params.comm_type == CommType.UDP:
            if self.is_server:
                self.call_create = lambda: cast(
                    asyncio.AbstractEventLoop, self.loop
                ).create_datagram_endpoint(
                    self.handle_new_connection,
                    local_addr=(host, port),
                )

dummy lambda does not return Coroutine as declared [CODE CHANGE]

transport/transport.py:ModbusProtocol

  self.call_create: Callable[[], Coroutine[Any, Any, Any]] = lambda: None

None is not a Coroutine

Resolved by using None instead of the dummy lambda

  self.call_create: Callable[[], Coroutine[Any, Any, Any]] | None = None

and adding an assertion to

    async def transport_connect(self) -> bool:
        ...
        try:
            self.transport, _protocol = await asyncio.wait_for(
                self.call_create(),
                timeout=self.comm_params.timeout_connect,
            )
        except USEEXCEPTIONS as exc:

here and in some related occurrences

    async def transport_connect(self) -> bool:
        ...
        try:
            assert self.call_create is not None
            self.transport, _protocol = await asyncio.wait_for(
                self.call_create(),
                timeout=self.comm_params.timeout_connect,
            )
        except USEEXCEPTIONS as exc:

… on Python 3.8 happy

janiversen

Something to get you going.

A couple of general comments:

do not mix "Union", "Optional" and "|", decide what to use and use it
do not use "cast" unless there is a very good reason to do so

Once these parts are done, please ask for a new review.

janiversen · 2023-10-14T16:56:51Z

pymodbus/client/base.py

@@ -22,7 +22,7 @@ class ModbusBaseClient(ModbusClientMixin, ModbusProtocol):

    **Parameters common to all clients**:

-    :param framer: (optional) Modbus Framer class.
+    :param framer: Modbus Framer class.


but framer is an optional parameter.

Did you read my comment 3? If framer is optional and defaults to None then it should not be called unprotected in __init__:

self.framer = framer(ClientDecoder(), self)

I don't think None can be called.

No I do not read comments, I review code. All optional parameters are described as optional, and framer is and should be optional.

None is a legal value for framer at API level.

janiversen · 2023-10-14T16:58:32Z

pymodbus/client/base.py


    def __init__(  # pylint: disable=too-many-arguments
        self,
-        framer: type[ModbusFramer] = None,
+        framer: type[ModbusFramer],


This changes the API, that is not something we do lightly !

I do not see the benefit of this change, please revert or explain why it is an advantage for the users.

Well, there is a type error in __init__ if this stays optional which should then be resolved in another way. How to initialize self.framer if the argument to framer not given and thus framer defaults to None?

That is another problem, which I think is handled at lower levels in the code.

Please do not change API, because this will cause every app to be changed.

Apart from that the type error is quite easy to solve without changing the parameter itself.

Ok, I did not see how to resolve that easily. Here I need your support.

janiversen · 2023-10-14T17:00:11Z

pymodbus/client/base.py

@@ -122,10 +122,10 @@ def __init__(  # pylint: disable=too-many-arguments
        self.transaction = DictTransactionManager(
            self, retries=retries, retry_on_empty=retry_on_empty, **kwargs
        )
-        self.reconnect_delay_current = self.params.reconnect_delay
+        self.reconnect_delay_current = cast(float, self.params.reconnect_delay)


both delay_current and reconnect_delay are (or should be float) so why the cast.

cast is an ugly beast, that preferable never should be used, it typically hides a fundamental problem, likely maybe in this case.

self.params.reconnect_delay defaults to None. Maybe it shouldn't.

Yes it should, that is the way to tell the lower system that reconnect is not used. That could of course be done differently, but please make 1 pull request for 1 problem, in this case "typing":

Ok, but if self.params.reconnect_delay can be None it should not be assigned to self.reconnect_delay_current which is declared to be a float in

self.reconnect_delay_current = self.params.reconnect_delay

janiversen · 2023-10-14T17:01:14Z

pymodbus/client/base.py

        """Handle received data

        returns number of bytes consumed
        """
        self.framer.processIncomingPacket(data, self._handle_response, slave=0)
        return len(data)

-    def callback_disconnected(self, _reason: Exception) -> None:
+    def callback_disconnected(self, _reason: Exception | None) -> None:


Can that really be None ? where is it called with None ?

See my comment 2

Again I review your code...All call I can see have an exception.

In transport/transport.py:transport_close there is a value.callback_disconnected(None) as I wrote in the PR comment.

janiversen · 2023-10-14T17:02:02Z

pymodbus/client/mixin.py

@@ -502,7 +502,7 @@ def read_fifo_queue(self, address: int = 0x0000, **kwargs: Any) -> ModbusRespons
    # code 0x2B sub 0x0D: CANopen General Reference Request and Response, NOT IMPLEMENTED

    def read_device_information(
-        self, read_code: int = None, object_id: int = 0x00, **kwargs: Any
+        self, read_code: Union[int, None] = None, object_id: int = 0x00, **kwargs: Any


Union ?? I think we should either use "Union" or "|" not both.

Use from __future__ import annotations to allow | to be used.

I will mark the other redundant comments Resolved.

pymodbus/server/simulator/http_server.py

janiversen · 2023-10-14T17:07:50Z

pymodbus/transport/transport.py

                return
+            host, port = host_serial, port_serial


This is just complicating things ! please use host, port directly.

See my comment 9

It is a not acceptable complication, that brings nothing.

Well it makes this type correct

janiversen · 2023-10-14T17:08:52Z

pymodbus/transport/transport.py

-    def init_setup_serial(self, host: str, _port: int) -> tuple[str, int]:
+    def init_setup_serial(
+        self, host: str, _port: int
+    ) -> tuple[str, int] | tuple[None, None]:


Can this really return None, None ?

Yes, also in comment 9.

janiversen · 2023-10-14T17:09:56Z

pymodbus/transport/transport.py

@@ -211,19 +214,25 @@ def init_setup_connect_listen(self, host: str, port: int) -> None:
        """Handle connect/listen handler."""
        if self.comm_params.comm_type == CommType.UDP:
            if self.is_server:
-                self.call_create = lambda: self.loop.create_datagram_endpoint(
+                self.call_create = lambda: cast(


That is wrong, call_create is NOT loop.

No, self.loop has to be a loop to call create_datagram_endpoint. See comment 10.

janiversen · 2023-10-14T17:10:37Z

pymodbus/transport/transport.py

@@ -289,7 +304,7 @@ def connection_made(self, transport: asyncio.BaseTransport):
        self.reset_delay()
        self.callback_connected()

-    def connection_lost(self, reason: Exception):
+    def connection_lost(self, reason: Exception | None) -> None:


Can that be called with None ?

See comment 11.

As MAKOMO explained, the default is in fact calling with None (via a lambda)

pymodbus/pymodbus/transport/transport.py

Line 162 in 2adff1f

self.call_create: Callable[[], Coroutine[Any, Any, Any]] = lambda: None

janiversen · 2023-10-14T17:17:43Z

I wrote "choose", but thinking a second time, the preference is to use "|" because that is the future.

MAKOMO · 2023-10-14T17:21:53Z

I wrote "choose", but thinking a second time, the preference is to use "|" because that is the future.

Yes

janiversen · 2023-10-14T17:47:47Z

I will wait to comment/review until you have made the requested changes, otherwise we keep commenting on the same things.

Please add me a reviewer when it is ready to be reviewed.

janiversen · 2023-10-14T18:04:05Z

I think the problem is that you try to solve the whole world in one go, that is difficult, even though I know the code quite good, I would have problems doing that.

Instead of giving up, submit a pull request with the simple changes, basically "| None", easy to review and easy to get merged...then you can focus on the more difficult items in smaller chunks.

But of course its all up to you.

(not sure why we started talking about this PR in my PR).

MAKOMO · 2023-10-14T18:18:41Z

I wrote "choose", but thinking a second time, the preference is to use "|" because that is the future.

Yes

I just replaced all Unions in client/mixing.py by | bars. Fine for mypy, but pylint complaints. Not supported under Python 3.8? Strange as transport/transport.py the | bars are accepted. Somehow the CI seems to treat different files in this project differently. I seem not to be the right person to bring this forward. Sorry.

janiversen · 2023-10-14T18:28:56Z

CI are not treating the files differently, but the files causing you problems might miss an import at the very top, to allow future changes.

janiversen · 2023-10-14T18:39:38Z

If I may be blunt, I do not think it's you as a person that's wrong....I actually find it refreshing to see your PR with interesting changes.

It seems you lack som python experience. The difference in the files are this "from future import annotations" and not CI or other things,

A similar problem was with the import variable "web", where it is correct that we want optional import, but there are different ways of doing it. transport_serial.py have a way that do not make mypy complain.

So back to what I started saying do not try to solve the whole world in one pull request, that is extremely difficult, use the "salami tactic", and make smaller easier pull requests. Just a recommendation.

Ps. I still think your goal is correct, and I hope you noted that I have not spoken against your changes just tried to guide you in the direction where we all benefit.

alexrudd2 · 2023-10-16T19:47:10Z

This situation is unfortunate. A well-meaning contributor and a well-meaning maintainer became frustrated at each other over some misunderstandings. I hope it may be possible to try again. :)

@janiversen I recommend you follow Marko's advice and read the pull request comments! He offers good analysis of the problems mypy finds, which are not apparent reading the code. Tthere are times he in fact caught something you missed. Reading the comments will make his intentions clear and your improvements more accurate.

@MAKOMO I recommend you follow Jan's advice and split this PR into smaller ones, starting with one change per PR. This makes it much easier to keep the comments near the code and review them.
I also recommend you setup the CI on your own fork, which will help you understand the different Python versions.

EDIT: (Sorry typo with MAKOMO)

alexrudd2 · 2023-10-16T19:54:02Z

As a first example, there was much discussion regarding Optional, Union, and |. Jan expressed frustration at inconsistency and prefers |, and Marko expressed frustration that | does not always work in the CI.

Using | instead of Optional[foo] or Union[foo, None] is PEP-0604, and was only added in Python3.10.

I presume Marko is using Python > 3.10 and so | always works. The CI tests against many versions, including 3.8 and 3.9. So it sometimes fails.

pymodbus/.github/workflows/ci.yml

Lines 31 to 36 in 2adff1f

    
           python: 
        
             - version: '3.8' 
        
             - version: '3.9' 
        
             - version: '3.10' 
        
             - version: '3.11' 
        
             - version: '3.12'

The answer is to use below for the cases where it fails.

from __future__ import annotations

janiversen · 2023-10-16T19:54:29Z

Yes it is very unfortunate! and I apologize but the pull request was big and I saw the same problems again and again, so I basically gave up on making a normal review which is also the reason for my not too polite (my apologies again) comment.

what especially concerned me was the inconsistent use of | and Option which was caused by not importing future (which I explained)

I hope he will come back, because his work was not bad, just needed finishing touches...which I hope I explained directly earlier.

alexrudd2 · 2023-10-16T20:04:17Z

When passionate Spain and methodical Germany can work peacefully together, the world will be amazed 😄 . I am going to try a few smaller PRs.

janiversen · 2023-10-16T20:43:19Z

heh, do not forget I am a real Viking (dane), living in Spain...I did live 5 years in Vienna if that counts.

Anyhow you are maintainer and THE typing specialist in this project, so your word on these subjects are what matters....I am just the old code guy 😀

alexrudd2 · 2023-10-16T23:32:36Z

See #1825 as an example

janiversen · 2023-10-17T07:07:06Z

Yes and see #1827, to see why not just accept everything.

Everything that makes the code more stable or faster are super, but changes that makes life harder for users of the library or complicates the code just to please mypy are at the very least something that should be discussed.

alexrudd2 · 2023-10-25T16:45:10Z

For an update on progress:

When the PR was created:

Found 87 errors in 13 files

Today:

Found 41 errors in 7 files

janiversen · 2023-10-28T20:09:48Z

Seems this PR is no longer updated, and have been replaced by a number of PR´s by @alexrudd2.

I am closing this, but the base of this is not bad !!! it just needed a bit of hand holding which have happened thanks to @alexrudd2. I am sorry for not being of more help, but mypy is to me working a lot against the nature of python. To be fair I am a C/C++ programmer so I am used to very strict typing etc, and python offered an interesting alternative.

MAKOMO added 6 commits October 14, 2023 16:38

eliminates implicit Optional

96e6fc9

make the framer argument to ModbusBaseClient mandatory to make pylint…

1563190

… on Python 3.8 happy

replace list and tuple by typing.List and typing.Tuple for Python3.8

9128fad

ruff formatting

fa75843

dont't exclude client/based from mypy

1288293

pymodbus/transport/serial_asyncio is gone

64ac7e0

janiversen requested changes Oct 14, 2023

View reviewed changes

MAKOMO closed this Oct 14, 2023

alexrudd2 reopened this Oct 16, 2023

janiversen closed this Oct 28, 2023

github-actions bot locked as resolved and limited conversation to collaborators Nov 8, 2023

eliminates implicit Optional #1817

eliminates implicit Optional #1817

Conversation

MAKOMO commented Oct 14, 2023

janiversen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janiversen Oct 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janiversen commented Oct 14, 2023

MAKOMO commented Oct 14, 2023

janiversen commented Oct 14, 2023

janiversen commented Oct 14, 2023

MAKOMO commented Oct 14, 2023

janiversen commented Oct 14, 2023

janiversen commented Oct 14, 2023

alexrudd2 commented Oct 16, 2023 • edited Loading

alexrudd2 commented Oct 16, 2023 • edited Loading

janiversen commented Oct 16, 2023 • edited Loading

alexrudd2 commented Oct 16, 2023

janiversen commented Oct 16, 2023 • edited Loading

alexrudd2 commented Oct 16, 2023

janiversen commented Oct 17, 2023

alexrudd2 commented Oct 25, 2023

janiversen commented Oct 28, 2023

janiversen Oct 14, 2023 •

edited

Loading

alexrudd2 commented Oct 16, 2023 •

edited

Loading

alexrudd2 commented Oct 16, 2023 •

edited

Loading

janiversen commented Oct 16, 2023 •

edited

Loading

janiversen commented Oct 16, 2023 •

edited

Loading