Add tests to support SensorReader refactoring

[benthorner]

This is the first part of a proposed change to refactor the SensorReader class so it's more flexible to use in a library.

Although the test coverage was reported as 100%, this is misleading as much of the SensorReader class is mocked when tests are run [^1]. Adding tests around SensorReader specifically means we can refactor with more confidence.

Since adding tests is a big change on its own, I thought it would be best to suggest it first, before the actual refactor.

[avaldebe]

Wow, really nice PR. I looked everywhere for a library to mock serial.Serial and came up empty. Thanks for introducing mock_serial to this project.

I would like to look in more detail into the Reader(AbstractContextManager) definition. In the current version I tried to define the internal API with typing.Protocol classes instead of abstract classes. Do you think it makes sense to move the Reader definition to pms/core/types.py?

It should take me a day or two to find the time to look at your PR in more detail. Please, remind me if it takes longer.

Thanks again, Á.

[benthorner]

@avaldebe thanks for taking a look 🙁.

I'm pleased you're keen on mock_serial - conscious it's a new dependency.

Regarding the types module, I agree it makes more sense for Reader to be a Protocol in there. I've tried this out and it sort of works, except there are a couple of problems:

• For some reason, the __call__ overloads in subclasses don't type check with __call__ on a Protocol: Signature of "__call__" incompatible with supertype "Reader".

• Moving the overloads to the Protocol is hard because one of them refers to RawData, which is in the reader module - doing the import would create a cyclic import.

It's possible to create a NamedTuple type for RawData and move the overloads. Even then, they still don't type check: Overloaded function implementation does not accept all possible arguments of signature 1 [misc].

I think the following could help:

1. Remove the overloads. As far as I can see they don't add any value since the actual __call__ method has raw as an optional argument, so it can already be used as a one or two argument method.

2. Create a protocol-like class for RawData in the types module (it has to inherit from NamedTuple[^1]). This makes it possible to fully type __call__ with -> Union[Iterator[ObsData], Iterator[RawData]].

3. [OPTIONAL / BREAKING] Remove the raw parameter to __call__ and always return RawData. Introduce a context manager helper that wraps Readers and knows how to emit ObsData.

Doing (1) unblocks making Reader a Protocol. Doing (2) unblocks fully typing the __call__ method for the Reader Protocol in types. (3) is just an extra step to unwind the typing of __call__ so it just does one thing.

I don't mind doing (1) and (2) as part of this PR. Doing (3) is too out-of-scope for me though.

What are your thoughts?

[^1]: Trying to do multiple inheritance of a Protocol and a NamedTuple fails; NamedTuple can only be used in a single-class inheritance context, apparently.

[avaldebe]

Hi @benthorner

I think the best is to keep this PR as it is. The ABC parent class (Reader) is not used anywhere else, so there is no real need to move it.

I'll merge your PR as it is. Please met me know what are you palling for your future PRs.

Cheers, Á.

[benthorner]

Oh nice thanks @avaldebe.

I'll try and get the next PR up soon with the actual refactoring in it. The change shouldn't be that big so it's probably easier if I just do it and then send the PR your way so you can see.

[benthorner]

Edit: while refactoring I went on a tangent to improve the variable names in one of the tests; not important really but here's the PR if you'd like to consider it as an extension of this one.