Add deprecation warning to inject #854
Conversation
|
@DominicOram is there anything else from your comments in #841 that you feel would be appropriate to document here? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #854 +/- ##
=======================================
Coverage 95.19% 95.19%
=======================================
Files 120 120
Lines 4976 4979 +3
=======================================
+ Hits 4737 4740 +3
Misses 239 239 ☔ View full report in Codecov by Sentry. |
| def my_plan() -> MsgGenerator: | ||
| detector = i22.saxs(connect_immediately=False) | ||
| # We need to connect via the stub instead of directly | ||
| # so that the RunEngine performs the connection in the | ||
| # correct event loop | ||
| yield from ops.ensure_connected(detector) | ||
| yield from bp.count([detector]) | ||
|
|
||
| RE(my_plan())) |
There was a problem hiding this comment.
I don't want to encourage this, I want to always have the device as a default argument.
This prevents any metrics/tracing/logging/documenting on the arguments of the plan, leads to N different plans that are just using a different device, makes it easier for the scope of the device to be managed incorrectly and cause issues (conditionally instantiating my device as mock/connecting my device?).
I think the Finder in GDA was a mistake that made every momentary decision to use it slightly easier and every subsequent attempt to extract what the hell was going on a lot harder.
I think it's risking making debugging a lot harder for a small benefit in verbosity, so I would like more attention paid to the above and comments about the benefits of doing it that way.
I'm prepared to be told I'm getting in the way of writing plans for this standpoint. I really don't like it.
There was a problem hiding this comment.
It also means we can call ensure_connected at the top of the plan and fail fast if any device won't be available, rather than after completing the first iteration of the fastest axis: and we can call connect on all of the devices in parallel rather than on each one as we encounter it.
There was a problem hiding this comment.
To be honest I broadly agree. Dependency injection is a good thing and, in my opinion, having lots of defaulted parameters is not a particularly terrible thing. Maybe we take this out for now and add it later if demand calls for it?
Fixes #845
Supersedes #839
Instructions to reviewer on how to test:
injectand confirm it raises a warning when run.Checks for reviewer
dodal connect ${BEAMLINE}