-
Notifications
You must be signed in to change notification settings - Fork 2
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
Use katcp-codec for encoding and decoding messages #90
Conversation
The minimum version is already listed in the README and setup.cfg, so I deleted this incorrect copy rather than trying to maintain it in yet another place.
katversion doesn't support the full range of version specifiers including beta versions. This also allows setup.py to finally be removed.
@ludwigschwardt I requested your review just for the packaging side of things. |
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.
Alas, poor katversion, I knew him...
else: | ||
mid = None | ||
mtype = cls._REVERSE_TYPE_SYMBOLS[clean[:1]] | ||
raise ValueError("message does not end with newline") |
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.
Why not KatcpSyntaxError
anymore?
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.
ValueErrors get turned into KatcpSyntaxErrors lower down anyway, so it seemed simpler to only raise ValueError (it's what ends up in msgs[0]
if the message is invalid) and not have to worry about catching KatcpSyntaxError.
src/aiokatcp/core.py
Outdated
raise ValueError("no message") | ||
if len(msgs) > 1 or parser.buffer_size > 0: | ||
raise ValueError("internal newline") | ||
if isinstance(msgs[0], Exception): |
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 could introduce a line like msg = msgs[0]
here to streamline the code a bit.
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.
I like that idea.
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.
Also, out of interest, how significant is the speedup with katcp-codec?
In microbenchmarks (particularly large requests like setting gains) it can be 5x, although in practice a lot of the time is spent elsewhere anyway, particularly converting Python objects (strings, floats) to/from raw bytes. |
I've made this as a beta release (1.10.0b1) because katcp-codec is still at beta.
I also ditched katversion rather than wait for ska-sa/katversion#31, so that I could make the release.