AFAICT_read_until doesn't support tuples as is (len(terminator) is wrong) and needs to be fixed to support them.

You are correct, I was so happy about figuring out that.endswith() accepts tuples that I've missed it ;-).

Ok, so I've figured out that if instead of passing an explicit tuple, I use*terminator arg, I can allow multiple terminators without having to actually change the call sites or having to add a compatibility hack. If you don't like that, I can revert to addingif not isinstance(terminator, tuple) or changing all the call sites.

Heh, wrong. I've just realized that stripping actually makes theerr.endswith() test wrong :-/.

I guess you need aread_until(*terminator, include_terminator=False|True)? Or just always include the terminator (returning a tuple for simpler use) and change all the call sites, perhaps that's slightly nicer APIwise.

Ok, I think it's ready now. I've noticed another late-night mistake, and fixed it. I've aimed for minimal change / keeping the code simple now.

I wonder if the whole thing would be simpler by having terminator be a regex instead, and returnmatch.groups()? This way you can just matchr"GS(?:<(\d+))>" instead of having to do two read_untils. (The slight extra cost of doing regex matches should be negligible compared to interacting with the subprocess.)

(If doing so, I would suggest having the argument be always acompiled regex object so that callers have to callre.compile themselves to make it completely clear that the argument is a regex.)

To be honest, I don't really want to spend more time on it and I don't think it's really important how it's implemented given it's only used in tests.

no worries

Thanks.