Change deserialization based around ComponentConfig instances

[thomascobb]

The YAML looks cleaner now, but there's some ugliness:

• DeviceSimulation sets Adapter.device and Adapter.raise_interrupt, mypy complains about the latter
• Component vs BaseComponent, mypy doesn't like returning a DeviceSimulation when the call signature says it wants a Component
• I find myself wanting to write class TCPServer(Server). Should Server, Adapter, Component be abstract base classes instead of protocols?
• Does the contents of configurable.py belong in component.py?

I expect there will be more issues as I convert the rest, but I wanted to PR early...

Fixes #4

[garryod]

• DeviceSimulation sets Adapter.device and Adapter.raise_interrupt, mypy complains about the latter

Could we eliminate the need for this by introspecting the base class when we generate the config class?

I.e. if Adapter is:

class Adapter:
    def __init__(device: Device, raise_interrupt: Callable[[],None]) -> None:
        ...

Then, could DerivedAdapterConfig be:

class DerivedAdapterConfig(AdapterConfig):
    def __call__(device: Device, raise_interrupt: Callable[[],None]) -> DerivedAdapter:
        return DerivedAdapter(device, raise_interrupt, *vars(self))

And DeviceSimulation could take adapters: Iterable[Callable[Device, Callable[[],None], Adapter]].

• Component vs BaseComponent, mypy doesn't like returning a DeviceSimulation when the call signature says it wants a Component

That seems rather odd and I have no immediate suggestions, though it may be a symptom of importing via typing_compat.

• I find myself wanting to write class TCPServer(Server). Should Server, Adapter, Component be abstract base classes instead of protocols?

I think I'm on board with this, having a concrete interface would be useful for both static type checking and config generation (see above). Would like to hear @callumforrester's thoughts though.

• Does the contents of configurable.py belong in component.py?

I don't believe so, I'm of the opinion that we make everything configurable but strongly encourage the inclusion of component configs which simplify the YAML. As such it doesn't make sense for it to be in the component module.

[thomascobb]

• DeviceSimulation sets Adapter.device and Adapter.raise_interrupt, mypy complains about the latter

Could we eliminate the need for this by introspecting the base class when we generate the config class?

The more I look at this, the more I would like to add these args into run_forever for each class. There is prior art for this in Server.run_forever:

    async def run_forever(
        self,
        on_connect: Callable[[], AsyncIterable[Optional[T]]],
        handler: Callable[[T], Awaitable[AsyncIterable[Optional[T]]]],
    ) -> None:

This would mean that __init__ for each class just takes config args, then run_forever takes the runtime context. This makes the "everything configurable" a lot easier, as they can be dataclasses, and we don't need separate Config classes anywhere. It also seems to fit with the pattern that asyncio classes require, __init__ stores args, then an async def will actually do the work.

What do you reckon?

• I find myself wanting to write class TCPServer(Server). Should Server, Adapter, Component be abstract base classes instead of protocols?

I think I'm on board with this, having a concrete interface would be useful for both static type checking and config generation (see above). Would like to hear @callumforrester's thoughts though.

I'll setup a meeting for next week to discuss

• Does the contents of configurable.py belong in component.py?

I don't believe so, I'm of the opinion that we make everything configurable but strongly encourage the inclusion of component configs which simplify the YAML. As such it doesn't make sense for it to be in the component module.

That makes sense

[garryod]

• DeviceSimulation sets Adapter.device and Adapter.raise_interrupt, mypy complains about the latter

Could we eliminate the need for this by introspecting the base class when we generate the config class?

The more I look at this, the more I would like to add these args into run_forever for each class. There is prior art for this in Server.run_forever:

    async def run_forever(
        self,
        on_connect: Callable[[], AsyncIterable[Optional[T]]],
        handler: Callable[[T], Awaitable[AsyncIterable[Optional[T]]]],
    ) -> None:

This would mean that __init__ for each class just takes config args, then run_forever takes the runtime context. This makes the "everything configurable" a lot easier, as they can be dataclasses, and we don't need separate Config classes anywhere. It also seems to fit with the pattern that asyncio classes require, __init__ stores args, then an async def will actually do the work.

What do you reckon?

I reckon that's probably the most viable solution, thinking about it, we don't actually want to be giving an Adapter the ability to poke at a Device or raise_interrupt prior to runtime anyway. It's likely worth re-working the lifetime_runnable module to reflect this.

[thomascobb]

Still need to fix the docs

[thomascobb]

@garryod this is ready to review

[codecov]

Codecov Report

Merging #16 (eda8414) into master (653cc5f) will increase coverage by 4.77%.
The diff coverage is 89.12%.

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
+ Coverage   88.09%   92.87%   +4.77%     
==========================================
  Files          43       46       +3     
  Lines        1462     1417      -45     
==========================================
+ Hits         1288     1316      +28     
+ Misses        174      101      -73     

Impacted Files	Coverage Δ
tickit/adapters/httpadapter.py	48.38% <43.47%> (-46.35%)	⬇️
tickit/devices/cryostream/cryostream.py	85.00% <55.00%> (-15.00%)	⬇️
tickit/core/components/system_simulation.py	71.42% <77.77%> (+14.28%)	⬆️
tickit/devices/source.py	81.81% <81.81%> (+3.24%)	⬆️
tickit/devices/cryostream/status.py	95.83% <83.33%> (+1.89%)	⬆️
tickit/core/components/component.py	97.61% <94.73%> (-2.39%)	⬇️
tickit/core/adapter.py	96.00% <95.00%> (-4.00%)	⬇️
tickit/adapters/composed.py	100.00% <100.00%> (ø)
tickit/adapters/epicsadapter.py	100.00% <100.00%> (+16.32%)	⬆️
tickit/adapters/servers/tcp.py	91.66% <100.00%> (+55.55%)	⬆️
... and 24 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 653cc5f...eda8414. Read the comment docs.

[thomascobb]

I think we're ready to merge unless you can see anything else to fix

[garryod]

Looks good to me, just going to change lifetime_runnable to runner since it's purpose has changed