[oliwenmandiamond]

BlueapiClient contains all of the logic for the client, devices and plans. This made the code hard to read as the file was very large and relied on circular dependencies.

I have split this logic up so we now have the addition of blueapi.client.protocols.py, blueapi.client.plan_cache.py, and blueapi.client.device_cache.py along with mirrored tests. This breaks the circular dependencies and makes the logic more modular and easier to maintain for future additions.

Additional doc strings and typing have been added to methods and classes.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.36%. Comparing base (ac3e6e5) to head (92ec084).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1667      +/-   ##
==========================================
+ Coverage   96.34%   96.36%   +0.02%     
==========================================
  Files          46       49       +3     
  Lines        3665     3686      +21     
==========================================
+ Hits         3531     3552      +21     
  Misses        134      134              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tpoliaw]

See also changes in #1338. They need updating after recent changes but might solve some of the concerns here.

[oliwenmandiamond]

See also changes in #1338. They need updating after recent changes but might solve some of the concerns here.

This PR is 9 months old and still needs to be rebased. It also only splits into cache rather than device cache and plan cache. This PR would make it more modular, e.g you could then add fixture cache, suspender cache, baseline cache in future PRs. It is also tied up with all other logic of generating the cache client side.

Does it not make more sense to merge this first and then that can be rebased on this and then that should reduce the size of that PR a lot as well? Then it can focus solely on the logic for generating client side cache rather than splitting logic at the same time?

[ZohebShaikh]

First of all Thank you for the PR.
I do like the test changes you have made.

The only logic that has changed is to remove the redundant name for Plan and DeviceRef as the model.name attribute shares the exact same one.

About this, Can we keep the behaviour as it is, because I think this is used to refer to a device
like bc.devices.stage.x error
If you really want this, Can this be put in a different PR and we can discuss the pros and cons of it there.

There is no integration tests for the scripting interface so I have created this
It might become more useful when scripting is doing a lot more stuff

[ZohebShaikh]

I didn't get the value in making this a protocol. I'm not against it, just want to know why you have added this. Is this out of habit of Java interfaces ?

[oliwenmandiamond]

Unfortunately I had to do this. The previous code had everything in one file which made it hard to understand the logic. When separating the logic out, their were a lot of circular references to each other. This was one of them. So the only way to break this circular dependency was to create a protocol

[oliwenmandiamond]

So for example, Plans and PlanCache takes in the Client and the Client has references to the plans. The DeviceCache has reference to the rest client and the rest client had reference to the DeviceCache. So we can break the circular dependencies with Protocols