Eliminate remaining implicit optional #1856
Conversation
| @@ -156,17 +156,21 @@ def __init__( | |||
| self.is_server = is_server | |||
| self.is_closing = False | |||
|
|
|||
| self.transport: asyncio.BaseTransport = None | |||
| self.loop: asyncio.AbstractEventLoop = None | |||
| self.loop: asyncio.AbstractEventLoop = None # type: ignore[assignment] | |||
There was a problem hiding this comment.
This is why I do not like the "| None", sometimes you use it, other times not...that its not really intuitive !
In my mind mypy is a bit stupid in this respect !
There was a problem hiding this comment.
This one was quite difficult because I could not figure out the intended usage of get_running_loop().
| self.transport: asyncio.BaseTransport = None | ||
| self.loop: asyncio.AbstractEventLoop = None | ||
| self.loop: asyncio.AbstractEventLoop = None # type: ignore[assignment] | ||
| self.transport: asyncio.BaseTransport | None = None |
There was a problem hiding this comment.
And here you use it....any maintainer will think why on earth is self.loop without None and this one is with None...both are later used in the same way.
There was a problem hiding this comment.
The later callers are different. Calling code checks self.transport != None in all places. But the loop() code is more complicated.
pymodbus/transport/transport.py
Outdated
| self.reconnect_task: asyncio.Task = None | ||
| self.reconnect_delay_current: float = 0.0 | ||
| self.reconnect_task: asyncio.Task | None = None | ||
| self.reconnect_delay_current: float | None = 0.0 |
There was a problem hiding this comment.
this cannot be None as far as I remember the code... 0.0 is not None.
There was a problem hiding this comment.
It's unclear what the intended value of self.reconnect_delay_current is when self.reconnect_delay == None.
I could not easily decide between 0.0 and None in that case, so I went with None for fewer code changes.
There was a problem hiding this comment.
self.reconnect_delay_current is a value between self.reconnect_delay and self.reconnect_delay_max. so setting it to self.reconnect_delay is probably the most generic way.
| @@ -58,12 +58,12 @@ def close(self, exc=None): | |||
| def write(self, data) -> None: | |||
| """Write some data to the transport.""" | |||
| self._write_buffer.append(data) | |||
| if not self.poll_task: | |||
| if not self.poll_task and self.sync_serial: | |||
There was a problem hiding this comment.
We cannot call write without sync_serial is created....this is the communication objects that comes from the connect.
There was a problem hiding this comment.
"cannot" != "do not"
To satisfy mypy it must be totally impossible to call incorrectly.
There was a problem hiding this comment.
this is a basic problem of mypy, it assumes all classes can be used randomly, which is NOT correct...and in reality one of my basic problems with mypy.
| self.async_loop.add_writer(self.sync_serial.fileno(), self._write_ready) | ||
|
|
||
| def flush(self) -> None: | ||
| """Clear output buffer and stops any more data being written""" | ||
| if not self.poll_task: | ||
| if not self.poll_task and self.sync_serial: |
The final changes are of course the most difficult. :) It's not possible to prove to |
Well that is your challenge, because we will not start to add code, that ensures that we call the methods correctly...that would and endless amendment of code that needs to be maintained and basically only slows down the library. |
|
May I politely suggest you isolate the type changes in another PR which would be approved fast, and leave the rest in this PR: |
|
Please accept, that I do not want to be negative !!! you do a lot good for the library, for which I am grateful, but mypy have some severe limitations (or assumptions) which do not work well with real life code, and together we need to find a compromise that do not add extra maintenance just to please mypy. |
Yep, I'll do that.
Yes, of course. Partly I opened this PR so we can discuss and find those compromises. (The easy ones already got merged) |
alexrudd2
force-pushed
the
implicit-optional
branch
from
d607222 to
ff77d3a
Compare
|
Rebased + force-pushed to reduce the diff and show the remaining problems. |
|
In I think we need to have a relaxed view on mypy and not the 100% which it seems you support. Because we know how our code works, there are a lot of situations where mypy tells this is an error, but due to the way the code is build this only happens if someone misuse the code. I am all in favour of using mypy where the code actually benefit from it, but e.g. adding code, which we have to maintain or deleting variables are not examples which I consider makes the code better. |
The stricter the checks, the more it finds. Here's an example where Mypy is currently at about 50% (because untyped defs are not checked, and other stricter options are not enabled). |
|
I disagree, mypy make one false assumption especially about our internal code, it assumes that all methods can be called anytime....but the internal code have sequences which we know and do not violate......like e.g. self.sync_serial = None in init is just to get it declared when the object is created, no calls will see that later. While I agree an API must be robust to invalid calls, our internals do not need to be just as robust, just like we in general do not validate parameters in internal calls. |
|
Your example is actually a good exammple to not make mypy stricter, the code works nice in our library but in the example it was used outside it the library. |
I think you are missing the point. OK, the bug was first found by external calling code. However, there was also an incorrect caller hiding in pymodbus code. It was not caught by the tests because it's a rarely-used error handler, and not the "happy path". except serial.SerialException as exc:
--- self._protocol.loop.call_soon(self._call_connection_lost, exc) # error
+++ self.async_loop.call_soon(self._call_connection_lost, exc) # fix
self.close() |
|
No I did not miss the point, but I think you missed the point that this is imported code and currently work in progress to be converted fully to our needs. |
|
Closing in favor of #1888 |
In most cases, expanding the type to include
| Noneis all that was required.In others, I needed to add a simple check:
AssertionErroror similar.I have split the changes into 3 commits, in case some are too much for one PR.