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

Added optional separator argument to TcpIo constructor #209

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

LeeHudsonDLS
Copy link
Contributor

When communicating with a simulated device using an ascii protocol I found that the StreamReader.read() method returns inconsistent number of bytes. Even the documentation alludes to this inconsistency:

If n is positive, this function try to read n bytes, and may return
less or equal bytes than requested, but at least one byte. If EOF was
received before any byte is read, this function returns empty byte
object.

As the majority of devices that communicate using an ascii based protocol rely on a termination character to determine the end of a message I have added this as an optional parameter in the tickit.adapters.io.TcpIo class.

@abbiemery
Copy link
Collaborator

Have you run the pre-commit linters and checkers on this?

Copy link
Collaborator

@abbiemery abbiemery left a comment

Choose a reason for hiding this comment

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

I've not checked it working with the tests yet but just some formatting things.

src/tickit/adapters/io/tcp_io.py Outdated Show resolved Hide resolved
src/tickit/adapters/io/tcp_io.py Outdated Show resolved Hide resolved
@LeeHudsonDLS
Copy link
Contributor Author

Sorry no I forgot to do the pre-checks, I have tested you changes though and it all seems to work thanks

@LeeHudsonDLS
Copy link
Contributor Author

@abbiemery Are you OK to approve this so it can be merged and released?

@callumforrester
Copy link
Contributor

Power cycling CI

@callumforrester
Copy link
Contributor

@LeeHudsonDLS Apologies, some of the CI was not running before

image

I have now fixed it.

Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

Looks good, only a couple of small items.

src/tickit/adapters/io/tcp_io.py Outdated Show resolved Hide resolved
self.host = host
self.port = port
self.separator = separator
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I'm not sure the name separator is completely right, maybe terminator? Or just add a comment explaining what it does, it's not immediately obvious to me from the name (unlike host and port). Not bothered by that enough to block merge though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Callum, i'm not too sure about this name either, the reason I chose separator is because that's the name of the parameter in StreamReader.readUntil. I guess in this context it really is a terminator though. It probably makes sense to change it, i'll do that now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants