-
Notifications
You must be signed in to change notification settings - Fork 8
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
Unify p38 and i22, swap out with env variables #678
base: main
Are you sure you want to change the base?
Conversation
89cfdd6
to
d02275e
Compare
122e734
to
655684a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #678 +/- ##
==========================================
- Coverage 94.44% 94.34% -0.10%
==========================================
Files 114 115 +1
Lines 4588 4561 -27
==========================================
- Hits 4333 4303 -30
- Misses 255 258 +3 ☔ View full report in Codecov by Sentry. |
type checking fails due to dodal not catching up to ophyd-async changes |
863806f
to
703a2fc
Compare
codecov for live beamline connectivity does not make that much sense, please let it merge |
I need this for #673 |
from @DiamondJoseph
|
@coretl would an addition of a 'lab' flag be in the scope for the device instantiation factory decorator? |
need to delete LAB_NAME from the CLI, and make it all optimized for testability |
src/dodal/beamlines/i22.py
Outdated
def linkam( | ||
wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False | ||
) -> Linkam3: | ||
return device_instantiation( | ||
Linkam3, | ||
"linkam", | ||
"-EA-TEMPC-05", | ||
"-EA-TEMPC-05" if IS_LAB else "-EA-TEMPC-05", |
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.
split devices like saxs
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.
???I do not understand
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.
if IS_LAB:
return this_device()
else:
return that_device()
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.
I do not agree that it's more readable, but I won't argue, changing now
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.
done
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.
You've done
if IS_LAB:
return this_device()
return that_device()
Which is functionally the same, but harder to read.
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.
Which is functionally the same, but harder to read.
that's a purely stylistic opinion, usefulness of else
is a debated point among software engineers
at the core the still not-agreed-on-implementation detail is where to put the i22-p38 mapping? when we read in the env in the CLI we need to check if the BEAMLINE belongs to the lab set and then load the |
Which CLI, dodal's? What I would expect is if you call |
ok fair rnough |
09b25eb
to
73fcee4
Compare
@callumforrester please weigh in, me and @DiamondJoseph got stuck update: this was confirmed as desired behavior: |
c8012ee
to
7d48404
Compare
_LAB_NAME = "p38" | ||
|
||
IS_LAB = BL == _LAB_NAME |
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.
_LAB_NAME = "p38" | |
IS_LAB = BL == _LAB_NAME | |
IS_LAB = BL == "p38" |
closer to i03/s03
7971fdb
to
02eb4d9
Compare
0dbd8a4
to
cfe97bf
Compare
Fixes #502
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}