77 multiple commands from a single message #82
Conversation
Codecov Report
@@ Coverage Diff @@
## master #82 +/- ##
==========================================
+ Coverage 98.58% 98.59% +0.01%
==========================================
Files 69 70 +1
Lines 2117 2141 +24
==========================================
+ Hits 2087 2111 +24
Misses 30 30
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
(Fixes flake8 failures)
|
Currently, should any of the sub-messages/commands raise an interrupt, the interrupt is only raised after all the commands have been executed. This means that a list of commands sent in a single message will not necessarily result in the same behavior as if each command was sent in its own message, despite being executed in the same order. This could be fixed by #84. |
| return results | ||
|
|
||
| @staticmethod | ||
| async def _get_response_and_interrupt_from_individual_results( |
There was a problem hiding this comment.
_collect_responses might be a nice shortform alternative
| for response_gen in individual_responses | ||
| async for response in response_gen | ||
| ] | ||
| response = functools.reduce(lambda a, b: a + b, response_list) |
There was a problem hiding this comment.
Might be nice to use something along the lines of response_deliminator.join(response_list) here, as it is more readable and allows us to define a response deliminator
There was a problem hiding this comment.
We did consider this. Whilst it's conceivable that a response where the individual messages are separated in some specific format could be required, we can't really think of one; so perhaps just add this feature if/when it's needed? We also would have to ensure that both delimiters are passed in as the same type - how much complication could this add?
There was a problem hiding this comment.
I don't think it would add any complication if the line were literally
response = "".join(response_list)which, I agree with @garryod, is more readable. You could also have
response = sum(response_list)I generally use functools.reduce when there aren't alternatives like this.
There was a problem hiding this comment.
I think response = "".join(response_list) may introduce typing issues since responses are of type AnyStr and we would need b"".join(...) sometimes?
Also, I don't think sum works with str or bytes?
There was a problem hiding this comment.
sum is literally functools.reduce(lambda a, b: a + b, collection), but I think that may be less readable in this case.
I think you're right about join too, so maybe leave as is
There was a problem hiding this comment.
Afaik sum cannot be used for string types, even with start="some_str". However, the fact that reduce(lambda a, b: a + b, response_list) is allowed by mypy is actually erroneous for response_list: list[Union[str, bytes]] as there is no implementation of __add__(self, other) for self: str & other: bytes or vice-versa, as such, we would need to type the input as Union[list[str], list[bytes]] or use a TypeVar to achieve the same (preferable), at which point ensuring both deliminators and the messages are the same type is fairly trivial.
There was a problem hiding this comment.
@callumforrester not quite sure why it doesn't like strings, but here we are
>>> sum(["a", "b", "c"])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for +: 'int' and 'str'
>>> sum(["a", "b", "c"], start="")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: sum() can't sum strings [use ''.join(seq) instead]
tests/adapters/interpreters/wrappers/test_splitting_interpreter.py
Outdated
Show resolved
Hide resolved
Would you like to block this merge until #84 is in place and we can replicate the "correct" behaviour, or merge early and track it with an issue? |
|
As discussed, I think converting this to a generic |
I suggest merging now and creating a new issue for the next part. |
| indicating whether an interrupt should be raised by the adapter. | ||
| """ | ||
| # re.split(...) can contain empty strings and None - we discard these | ||
| individual_messages = [_ for _ in re.split(self.delimiter, message) if _] |
There was a problem hiding this comment.
Would be good to avoid using _ as a variable name. Also, would be best to explicitly check not None as other values may be falsey
| individual_messages = [_ for _ in re.split(self.delimiter, message) if _] | |
| individual_messages = [message for message in re.split(self.delimiter, message) if message is not None] |
There was a problem hiding this comment.
As well as None, re.split can also have additional empty strings in its return value, e.g.:
>>> re.split("pattern", "pattern at beginning")
['', ' at beginning']
If "" is to be recognised as a command with a response, we don't want to introduce extra empty strings into the list of messages to be handled. I am currently relying on the falsyness of "" to remove any extra empty strings introduced, whilst handling the case of completely empty message separately. Can we guarantee that an empty message, when sent to a device, will never require a response? I think the answer is no, since I currently seem to need this for the PMAC (DiamondLightSource/tickit-devices#2), though in that case the message is only empty once a header is removed with a SplittingInterpreter.
There was a problem hiding this comment.
Good point! I hadn't considered the pattern at beginning case, or the need to handle zero-length messages. However, it seems to me that only removing instances of None and foregoing re-adding an zero length message in empty case would achieve exactly what you're looking for with the added ability to send zero length messages in the middle of concatenated messages, e.g:
re.split(";", "hello;;world")Giving:
['hello', '', 'world']And the zero length case:
re.split(";", "")Giving:
['']What are your thoughts on this?
There was a problem hiding this comment.
I'm not sure this solves my problem (I may have misunderstood). I'll try to give an example:
Suppose you have a device that accepts 3 commands, "", cmd1, and cmd2, and needs to respond to each with "0", "1", and "2", respectively. Suppose also that it is sent messages that look like "" or "/cmd1/cmd2" etc.. I think wrapping a CommandInterpreter with a SplittingInterpreter using message_delimiter="/" (and, say, response_delimiter="") makes sense here. If it is sent the message "/cmd1/cmd2" we would like it to pass on each of ["cmd1", "cmd2"] to the wrapped interpreter, and eventually send back "12"; doing it your way would we instead be passing on ["", "cmd1, "cmd2"] and sending back "012"?
I realise this is quite contrived, my 'real' example is related to DiamondLightSource/tickit-devices#10, but this has extra complications.
There was a problem hiding this comment.
In this case, where we wish to discard a leading delimiter we would need to wrap the SplittingInterpreter with a BeheadingInterpreter which chops off the first character. This still allows us to handle devices which receive a zero-length command at the beginning
There was a problem hiding this comment.
That does work... A slightly more complicated setup:
- The device accepts the commands
#,#x, with x in {1,...8} andP - The device is given messages consisting of several 'words' separated by
" " - Each word is either a single command or two commands merged together, but only as
#xP(notPPorP#xetc.) - The number of responses must be equal to the number of words
- The device must respond to a empty message
- Say the response to
#xis an empty message, the response toPis"0"and the response to#is"1" - For each message there must be a reply message consisting of all individual responses followed by, say
"/"
Some example messages, commands to be parsed and required replies:
"" -> [""] -> "/""#1P #2 P" -> ["#1", "P", "#2", "P"] -> "0//0/""P #2 #" -> ["P", "#2", "#"] -> "0//1/"
My approach to this was to wrap a CommandInterpreter with a SplittingInterpreter with message_delimiter="(#[1-8]?)" and response_delimiter="", then wrap that in another SplittingInterpreter with message_delimiter=" " and response_delimiter="/", but this only works with how I currently have the splitting set up.
There's a reasonable chance I'm over-complicating things...
There was a problem hiding this comment.
Complex examples such as this should probably be handled with a more complex interpreter. For now, I think we should go with the is not None suggestion, even though it may potentially lead to unexpected behavior in some cases.
|
Note that is is now only a partial fix for #77. A full fix requires a more complex interpreter. |
| # If splitting/filtering gives no sub-messages, pass on an empty message | ||
| individual_messages = ( | ||
| individual_messages if individual_messages else [type(message)()] | ||
| ) |
There was a problem hiding this comment.
If only filtering out None this will never be invoked so can be removed
|
Converted to draft because it requires #89 to be merged first. |
|
|
||
|
|
||
| async def wrap_messages_as_async_iterable( | ||
| messages: List[AnyStr], |
There was a problem hiding this comment.
This could be made more generic as an Iterable
| """Wraps a message in an asynchronous iterable. | ||
|
|
||
| Args: | ||
| message (AnyStr): A singular message. |
There was a problem hiding this comment.
This doc string doesn't reflect the actual function arguments
| A tuple of the asynchronous iterable of reply messages and a flag | ||
| indicating whether an interrupt should be raised by the adapter. | ||
| """ | ||
| # re.split(...) can contain instances of None - we discard these |
There was a problem hiding this comment.
| # re.split(...) can contain instances of None - we discard these |
| (resp, interrupt) = await self._collect_responses(results) | ||
|
|
||
| return resp, interrupt |
There was a problem hiding this comment.
| (resp, interrupt) = await self._collect_responses(results) | |
| return resp, interrupt | |
| return await self._collect_responses(results) |
Fixes #77