-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added indexing to handle tuple #468
Conversation
The tests are failing now because it is testing it with the old version of dodal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests would be useful, even they are just copy/pastes of the dodal tests for now, we can work on consolidation later.
src/blueapi/core/context.py
Outdated
for device in devices.values(): | ||
self.device(device) | ||
|
||
num_failed = len(exceptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we register devices that had exceptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exceptions
is literally a dictionary of str
-> Exception
rather than str
-> Device
. There is nothing to register.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has the signature of make_all_devices changed? Seems it used to return dict[str, Device]
but now returns tuple[dict[str, Device], dict[str, Exception]]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It changes in the corresponding dodal PR, see description: DiamondLightSource/dodal#550
da28b14
to
d2ecd6e
Compare
Tests will pass following a dodal release |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #468 +/- ##
==========================================
+ Coverage 90.43% 90.46% +0.02%
==========================================
Files 40 40
Lines 1694 1699 +5
==========================================
+ Hits 1532 1537 +5
Misses 162 162 ☔ View full report in Codecov by Sentry. |
src/blueapi/core/context.py
Outdated
# If exceptions have occurred, we log them but we do not make blueapi | ||
# fall over | ||
if len(exceptions) > 0: | ||
LOGGER.warning(f"{len(exceptions)} device(s) failed to connect") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
LOGGER.warning(f"{len(exceptions)} device(s) failed to connect") | |
LOGGER.warning(f"{len(exceptions)} exceptions while attempting to connect") |
As discussed, any Errors from method signatures also get inserted
2aab175
to
b695858
Compare
closes #444
With PR for dodal at DiamondLightSource/dodal#550