Repository: PandABlocks/PandABlocks-client

Issue:
look at changing return value of send
to always be a list.
this may reduce the amount of code checking the type of the return.

Discussion:

Patch:
diff --git a/.github/workflows/code.yml b/.github/workflows/code.yml
index 0768ee0..4cfab26 100644
--- a/.github/workflows/code.yml
+++ b/.github/workflows/code.yml
@@ -25,7 +25,7 @@ jobs:
         run: |
           env
           pip install pipenv
-          pipenv install --dev --deploy && pipenv graph
+          pipenv install --dev --deploy --python $(which python) && pipenv graph
 
       - name: Create Sdist and Wheel
         # for reproducible builds set SOURCE_DATE_EPOCH to the date of the last commit
diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml
index a0be5d9..0ec5c24 100644
--- a/.github/workflows/docs.yml
+++ b/.github/workflows/docs.yml
@@ -26,7 +26,7 @@ jobs:
       - name: Install Python Dependencies
         run: |
           pip install pipenv
-          pipenv install --dev --deploy && pipenv graph
+          pipenv install --dev --deploy --python $(which python) && pipenv graph
 
       - name: Deploy index
         # We pin to the SHA, not the tag, for security reasons.
diff --git a/pandablocks/commands.py b/pandablocks/commands.py
index bf334cb..a3b6ddd 100644
--- a/pandablocks/commands.py
+++ b/pandablocks/commands.py
@@ -4,8 +4,30 @@ from typing import Dict, Generic, List, Tuple, TypeVar, Union
 from .responses import FieldType
 
 T = TypeVar("T")
-# One or more lines to send
-Lines = Union[bytes, List[bytes]]
+
+
+@dataclass
+class RawResponse:
+    """A helper class representing a list of lines returned from PandA
+
+    No newlines
+    """
+
+    lines: List[str]
+    is_multiline: bool = False
+
+    @property
+    def line(self) -> str:
+        """Check we are not multiline and return the line"""
+        assert not self.is_multiline, f"{self} has many lines"
+        return self.lines[0]
+
+    @property
+    def multiline(self) -> List[str]:
+        """Return the lines, processed to remove markup"""
+        assert self.is_multiline, f"{self} is not multiline"
+        # Remove the ! and . markup
+        return [line[1:] for line in self.lines[:-1]]
 
 
 class CommandException(Exception):
@@ -16,28 +38,17 @@ class CommandException(Exception):
 class Command(Generic[T]):
     """Abstract baseclass for all `ControlConnection` commands to be inherited from"""
 
-    def lines(self) -> Lines:
+    def lines(self) -> List[str]:
         """Return lines that should be sent to the PandA, with no newlines in them"""
         raise NotImplementedError(self)
 
-    def response(self, lines: Lines) -> T:
+    def response(self, raw: RawResponse) -> T:
         """Create a response from the lines received from the PandA"""
         raise NotImplementedError(self)
 
-    def ok_if(self, ok, lines: Lines):
-        """If not ok then raise a suitable `CommandException`"""
-        if not ok:
-            msg = f"{self} ->"
-            if isinstance(lines, list):
-                for line in lines:
-                    msg += "\n    " + line.decode()
-            else:
-                msg += " " + lines.decode()
-            raise CommandException(msg)
-
 
 @dataclass
-class Get(Command[Lines]):
+class Get(Command[Union[str, List[str]]]):
     """Get the value of a field or star command.
 
     Args:
@@ -45,25 +56,25 @@ class Get(Command[Lines]):
 
     For example::
 
-        Get("PCAP.ACTIVE") -> b"1"
-        Get("SEQ1.TABLE") -> [b"1048576", b"0", b"1000", b"1000"]
-        Get("*IDN") -> b"PandA 1.1..."
+        Get("PCAP.ACTIVE") -> "1"
+        Get("SEQ1.TABLE") -> ["1048576", "0", "1000", "1000"]
+        Get("*IDN") -> "PandA 1.1..."
     """
 
     field: str
 
-    def lines(self) -> Lines:
-        return f"{self.field}?".encode()
+    def lines(self) -> List[str]:
+        return [f"{self.field}?"]
 
-    def response(self, lines: Lines) -> Lines:
-        """The value that was requested as a byte string. If it is multiline
-        then it will be a list of byte strings"""
-        if not isinstance(lines, list):
-            # We got OK =value
-            self.ok_if(lines.startswith(b"OK ="), lines)
-            return lines[4:]
+    def response(self, raw: RawResponse) -> Union[str, List[str]]:
+        """The value that was requested as a string. If it is multiline then it
+        will be a list of strings"""
+        if raw.is_multiline:
+            return raw.multiline
         else:
-            return lines
+            # We got OK =value
+            assert raw.line.startswith("OK =")
+            return raw.line[4:]
 
 
 @dataclass
@@ -83,27 +94,35 @@ class Put(Command[None]):
     field: str
     value: Union[str, List[str]] = ""
 
-    def lines(self) -> Lines:
+    def lines(self) -> List[str]:
         if isinstance(self.value, list):
             # Multiline table with blank line to terminate
-            return (
-                [f"{self.field}<".encode()] + [v.encode() for v in self.value] + [b""]
-            )
+            return [f"{self.field}<"] + self.value + [""]
         else:
-            return f"{self.field}={self.value}".encode()
+            return [f"{self.field}={self.value}"]
 
-    def response(self, lines: Lines):
-        self.ok_if(lines == b"OK", lines)
+    def response(self, raw: RawResponse):
+        assert raw.line == "OK"
 
 
 class Arm(Command[None]):
     """Arm PCAP for an acquisition by sending ``*PCAP.ARM=``"""
 
-    def lines(self) -> Lines:
-        return b"*PCAP.ARM="
+    def lines(self) -> List[str]:
+        return ["*PCAP.ARM="]
+
+    def response(self, raw: RawResponse):
+        assert raw.line == "OK"
 
-    def response(self, lines: Lines):
-        self.ok_if(lines == b"OK", lines)
+
+class Disarm(Command[None]):
+    """Disarm PCAP, stopping acquisition by sending ``*PCAP.DISARM=``"""
+
+    def lines(self) -> List[str]:
+        return ["*PCAP.DISARM="]
+
+    def response(self, raw: RawResponse):
+        assert raw.line == "OK"
 
 
 class GetBlockNumbers(Command[Dict[str, int]]):
@@ -114,18 +133,17 @@ class GetBlockNumbers(Command[Dict[str, int]]):
         GetBlockNumbers() -> {"LUT": 8, "PCAP": 1, ...}
     """
 
-    def lines(self) -> Lines:
-        return b"*BLOCKS?"
+    def lines(self) -> List[str]:
+        return ["*BLOCKS?"]
 
-    def response(self, lines: Lines) -> Dict[str, int]:
+    def response(self, raw: RawResponse) -> Dict[str, int]:
         """The name and number of each block type in a dictionary,
         alphabetically ordered"""
-        blocks = {}
-        assert isinstance(lines, list), f"Expected list of Blocks, got {lines!r}"
-        for line in lines:
+        blocks_list = []
+        for line in raw.multiline:
             block, num = line.split()
-            blocks[block.decode()] = int(num)
-        return {block: num for block, num in sorted(blocks.items())}
+            blocks_list.append((block, int(num)))
+        return dict(sorted(blocks_list))
 
 
 @dataclass
@@ -142,16 +160,15 @@ class GetFields(Command[Dict[str, FieldType]]):
 
     block: str
 
-    def lines(self) -> Lines:
-        return f"{self.block}.*?".encode()
+    def lines(self) -> List[str]:
+        return [f"{self.block}.*?"]
 
-    def response(self, lines: Lines) -> Dict[str, FieldType]:
+    def response(self, raw: RawResponse) -> Dict[str, FieldType]:
         """The name and `FieldType` of each field in a dictionary, ordered
         to match the definition order in the PandA"""
         unsorted: Dict[int, Tuple[str, FieldType]] = {}
-        assert isinstance(lines, list), f"Expected list of Fields, got {lines!r}"
-        for line in lines:
-            name, index, type_subtype = line.decode().split(maxsplit=2)
+        for line in raw.multiline:
+            name, index, type_subtype = line.split(maxsplit=2)
             unsorted[int(index)] = (name, FieldType(*type_subtype.split()))
         # Dict keeps insertion order, so insert in the order the server said
         fields = {name: field for _, (name, field) in sorted(unsorted.items())}
@@ -203,13 +220,9 @@ class Raw(Command[List[str]]):
 
     inp: List[str]
 
-    def lines(self) -> Lines:
-        return [line.encode() for line in self.inp]
+    def lines(self) -> List[str]:
+        return self.inp
 
-    def response(self, lines: Lines) -> List[str]:
+    def response(self, raw: RawResponse) -> List[str]:
         """The lines that PandA responded, including the multiline markup"""
-        if isinstance(lines, List):
-            # Add the multiline markup back in...
-            return [f"!{line.decode()}" for line in lines] + ["."]
-        else:
-            return [lines.decode()]
+        return raw.lines
diff --git a/pandablocks/connections.py b/pandablocks/connections.py
index 5d6eaf8..29d4809 100644
--- a/pandablocks/connections.py
+++ b/pandablocks/connections.py
@@ -6,7 +6,7 @@ from typing import Any, Callable, Deque, Iterator, List, Optional, Tuple
 
 import numpy as np
 
-from .commands import Command, Lines
+from .commands import Command, CommandException, RawResponse
 from .responses import (
     Data,
     EndData,
@@ -106,15 +106,21 @@ class ControlConnection:
 
     def __init__(self):
         self._buf = Buffer()
-        self._lines: List[bytes] = []
+        self._lines: List[str] = []
         self._commands: Deque[Command] = deque()
 
-    def _pop_command_response(self, lines: Lines) -> Tuple[Command, Any]:
+    def _pop_command_response(self, raw: RawResponse) -> Tuple[Command, Any]:
         command = self._commands.popleft()
         try:
-            response = command.response(lines)
+            response = command.response(raw)
         except Exception as e:
-            response = e
+            msg = f"{command} ->"
+            if raw.is_multiline:
+                for line in raw.multiline:
+                    msg += "\n    " + line
+            else:
+                msg += " " + raw.line
+            response = CommandException(msg).with_traceback(e.__traceback__)
         return command, response
 
     def receive_bytes(self, received: bytes) -> Iterator[Tuple[Command, Any]]:
@@ -123,37 +129,40 @@ class ControlConnection:
         command that triggered this response"""
         self._buf += received
         is_multiline = bool(self._lines)
-        for line in self._buf:
+        for line_b in self._buf:
+            line = line_b.decode()
             if not is_multiline:
                 # Check if we need to switch to multiline mode
-                is_multiline = line.startswith(b"!") or line == b"."
-            if line == b".":
-                # End of multiline mode, return what we've got
-                yield self._pop_command_response(self._lines)
-                self._lines = []
-                is_multiline = False
-            elif is_multiline:
+                is_multiline = line.startswith("!") or line == "."
+            if is_multiline:
                 # Add a new line to the buffer
-                assert line.startswith(b"!"), (
-                    "Multiline response %r doesn't start with !" % line
-                )
-                self._lines.append(line[1:])
+                self._lines.append(line)
+                if line == ".":
+                    # End of multiline mode, return what we've got
+                    yield self._pop_command_response(
+                        RawResponse(self._lines, is_multiline)
+                    )
+                    self._lines = []
+                    is_multiline = False
+                else:
+                    # Check a correctly formatted response
+                    assert line.startswith("!"), (
+                        "Multiline response %r doesn't start with !" % line
+                    )
             else:
                 # Single line mode
                 assert not self._lines, (
                     "Multiline response %s not terminated" % self._lines
                 )
-                yield self._pop_command_response(line)
+                yield self._pop_command_response(RawResponse([line]))
 
     def send(self, command: Command) -> bytes:
         """Tell the connection you want to send an event, and it will return
         some bytes to send down the network
         """
         self._commands.append(command)
-        lines = command.lines()
-        if isinstance(lines, list):
-            lines = b"\n".join(lines)
-        return lines + b"\n"
+        text = "\n".join(command.lines()) + "\n"
+        return text.encode()
 
 
 class DataConnection:
diff --git a/pandablocks/state.py b/pandablocks/state.py
index 2dda781..778250c 100644
--- a/pandablocks/state.py
+++ b/pandablocks/state.py
@@ -34,10 +34,10 @@ class State:
         lines = self._client.send(Get(command))
 
         # send may return a single bytes or a list of bytes
-        if isinstance(lines, bytes):
-            result = [lines.decode()]
+        if isinstance(lines, list):
+            result = lines
         else:
-            result = [line.decode() for line in lines]
+            result = [lines]
         return result
 
     def save(self) -> List[str]:


Test patch:
diff --git a/tests/test_asyncio.py b/tests/test_asyncio.py
index dd4ca1b..20043f7 100644
--- a/tests/test_asyncio.py
+++ b/tests/test_asyncio.py
@@ -11,7 +11,7 @@ async def test_asyncio_get(dummy_server_async):
     dummy_server_async.send.append("OK =something")
     async with AsyncioClient("localhost") as client:
         response = await asyncio.wait_for(client.send(Get("PCAP.ACTIVE")), timeout=1)
-    assert response == b"something"
+    assert response == "something"
     assert dummy_server_async.received == ["PCAP.ACTIVE?"]
 
 
diff --git a/tests/test_blocking.py b/tests/test_blocking.py
index 7fb30d8..f9adf1b 100644
--- a/tests/test_blocking.py
+++ b/tests/test_blocking.py
@@ -7,8 +7,8 @@ from pandablocks.commands import CommandException, Get, Put
 def test_blocking_get(dummy_server_in_thread):
     dummy_server_in_thread.send.append("OK =something")
     with BlockingClient("localhost") as client:
-        response = client.send(Get("PCAP.ACTIVE"))
-    assert response == b"something"
+        response = client.send(Get("PCAP.ACTIVE"), timeout=1)
+    assert response == "something"
     assert dummy_server_in_thread.received == ["PCAP.ACTIVE?"]
 
 
@@ -16,7 +16,7 @@ def test_blocking_bad_put_raises(dummy_server_in_thread):
     dummy_server_in_thread.send.append("ERR no such field")
     with BlockingClient("localhost") as client:
         with pytest.raises(CommandException) as cm:
-            client.send(Put("PCAP.thing", 1))
+            client.send(Put("PCAP.thing", 1), timeout=1)
         assert str(cm.value) == "Put(field='PCAP.thing', value=1) -> ERR no such field"
     assert dummy_server_in_thread.received == ["PCAP.thing=1"]
 
diff --git a/tests/test_pandablocks.py b/tests/test_pandablocks.py
index 2369d62..2ea3837 100644
--- a/tests/test_pandablocks.py
+++ b/tests/test_pandablocks.py
@@ -19,7 +19,7 @@ def test_connection_gets_split_value():
     cmd = Get("PCAP.ACTIVE")
     assert conn.send(cmd) == b"PCAP.ACTIVE?\n"
     assert list(conn.receive_bytes(b"OK =1")) == []
-    assert list(conn.receive_bytes(b"\nAnySpamWeLike")) == [(cmd, b"1")]
+    assert list(conn.receive_bytes(b"\nAnySpamWeLike")) == [(cmd, "1")]
 
 
 def test_connection_gets_muliline():
@@ -28,7 +28,7 @@ def test_connection_gets_muliline():
     assert conn.send(cmd) == b"SEQ1.TABLE?\n"
     assert list(conn.receive_bytes(b"!1048576\n!0\n!10")) == []
     assert list(conn.receive_bytes(b"00\n!1000\n.\n")) == [
-        (cmd, [b"1048576", b"0", b"1000", b"1000"])
+        (cmd, ["1048576", "0", "1000", "1000"])
     ]