ISISComputingGroup / lewis-ess

Let's write intricate simulators!
GNU General Public License v3.0
21 stars 19 forks source link

Fix byte str mixed #295

Closed John-Holt-Tessella closed 2 years ago

John-Holt-Tessella commented 3 years ago

We occasionally see an issue where we get a type error mixing bytes and string. Although we can not see where the error is coming from we don't want push to fall over so:

  1. catch the exception and report it.
  2. convert byte terminator to string before concaternating it
mattclarke commented 3 years ago

@DominicOram John's left now, right? Shall we close this?

DominicOram commented 3 years ago

He has, yes. I want to remind myself of what this was for before closing it. I'll add it to my list of things to look at.

chris-d-gregory commented 3 years ago

I understand why you are reluctant to approve this, and I don't know what the original reason for this modification is, but I wanted to comment here as we've done exactly the same thing at our site for the following case:

  1. Go through and change all your return values to bytes - including those for which no special handling is needed - which might be a lot of otherwise unnecessary code. Then return an encoded out terminator too so that the two can be concatenated when building the complete reply. This works with the released version of lewis.
  2. Make the modification in this PR and you only have to manually encode those problem return values and leave the rest as they are. The fact that this modification checks the type of the out terminator and the reply separately to see what needs encoding allows this to happen.

I accept that might not be a good enough reason to approve the PR.

DominicOram commented 2 years ago

I've changed it so that Lewis is as forgiving as possible now and added a test. I don't think docs are required as they'll basically boil down to "specify it in any type you want". @mattclarke and @chris-d-gregory look ok to you?

mattclarke commented 2 years ago

👍

chris-d-gregory commented 2 years ago

Yes, looks good.