-
Notifications
You must be signed in to change notification settings - Fork 949
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
Rework host/port and listener setup #1866
Conversation
See #1865. If the test that creates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are touching a very delicate part of the code.
Some of your does not seem correct at a first look,but might very well be correct.
I need a bit of time to review this one.
That is a valid specification, the definition is "socket://:" for serial port as sockets. |
It will remain a secret, how many test failures I had in my attempts 🤣 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Too fast, have to look at the whole PR:
The reason I tried changing things here to was to avoid this inelegant return of 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 Perhaps you have a cleaner way. |
Your last change looks ok....but you still have the change with strip and split, which I overlooked when I first approved the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
The refactored code is less nested, so I hope it is easier to understand.
It also solves a
mypy
error.