Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate more implicit optionals in transport #1873

Closed
wants to merge 6 commits into from

Conversation

alexrudd2
Copy link
Collaborator

Instead of

if foo [is not None]:
  foo = None

use

if hasattr(self, 'foo'):
  del foo

Also, revert #1859

pymodbus/transport/transport.py:255: error: "None" not callable [misc]

@@ -159,7 +159,11 @@ def __init__(
self.transport: asyncio.BaseTransport = None
self.loop: asyncio.AbstractEventLoop = None
self.recv_buffer: bytes = b""
self.call_create: Callable[[], Coroutine[Any, Any, Any]] = lambda: None

async def _noop():
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really do not like adding _noop methods (potentially quite a lot). Lambda is a perfectly legal one line function call.

Adding more lines of code, just to please mypy is not a good way!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only one!

(dev is down to 9 mypy errors with strict_optional = true)

Copy link
Collaborator

@janiversen janiversen Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the principle, we do not want to have extra code just to please mypy....of course assuming the original is correct and in accordance with the PEP

Copy link
Collaborator Author

@alexrudd2 alexrudd2 Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

is not in accordance with the revised PEP484.

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

would be, but of course it introduces other type errors with calling code.

I tried to come up with an async lambda => return None or similar, but could not find one.

pymodbus/transport/transport_serial.py Show resolved Hide resolved
@@ -38,7 +38,7 @@ def setup(self):

def close(self, exc: Exception | None = None) -> None:
"""Close the transport gracefully."""
if not self.sync_serial:
if not hasattr(self, 'sync_serial'):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see below please.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I understand your concern. Permitting sync_serial to be None means introducing more code to guard against calling it when None, so I found it cleanest to simply del the whole object.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or telling mypy to keep silent !!! we do not add code, just to please mypy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My experience is actually different a lot of the juniors do not understand nor agree with type checking at all....

There are many python developers who see the lack of type checking as a big advantage...being a C/C++/C# I am very used to strict type checking, but have also seen how it can make code a lot more complicated.

Personally I think the bridge (free use / type checking) python tries to build might end being a split into 2 products.

I like the freedom python allows, but see that mypy catches many problems....so for me a balance is the correct choice, meaning think if what mypy suggest mamés the code better if not ask mypy to stop complaining.

pymodbus/transport/transport.py Show resolved Hide resolved
@alexrudd2
Copy link
Collaborator Author

Closing in favor of #1888

@alexrudd2 alexrudd2 closed this Nov 12, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 23, 2023
@janiversen janiversen deleted the more-implicit-optional-transport branch December 7, 2023 08:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants