Beamline creation readme #323
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #323 +/- ##
==========================================
+ Coverage 89.97% 92.07% +2.10%
==========================================
Files 83 83
Lines 3171 3181 +10
==========================================
+ Hits 2853 2929 +76
+ Misses 318 252 -66 ☔ View full report in Codecov by Sentry. |
DiamondJoseph
force-pushed
the
beamline-creation-readme
branch
2 times, most recently
from
16f92f2
to
35c80b7
Compare
|
This change does enforce that Only device factory functions are defined in the beamlines package, and also enforces that beamlines either utilise device_instantiation or some other form of ensuring their methods return a singleton. Test coverage of new beamlines is already above the required patchset percentage just by inclusion in test_device_instantiation, which should reduce the barrier to entry of getting new beamlines added. |
DiamondJoseph
requested review from
dperl-dls,
callumforrester and
DominicOram
DiamondJoseph
force-pushed
the
beamline-creation-readme
branch
from
2d7ef7d
to
a258c40
Compare
|
|
||
| def test_device_creation(): | ||
| devices = make_all_devices(i24, fake_with_ophyd_sim=True) | ||
| @patch.object(dodal.utils, "get_beamline_name", return_value="i24") |
There was a problem hiding this comment.
I prefer the solution in #330 (review) - I know it's just a test, but reloading modules and deleting from sys feels extra messy to me - and it's not really necessary - all that's needed is to set the beamline_utils.BL and clear the devices like I did here 05ab615
There was a problem hiding this comment.
Now that 330 is merged I'm happy to just rebase on top of main again and discard this. Any thoughts on the rest of the changes?
There was a problem hiding this comment.
Yes, I think this is great! it'll be very helpful I'm sure. It looks detailed enough for most developers to me, hopefully that's right. But I think you'd get better feedback from someone who didn't write the thing in the first place, haha
There was a problem hiding this comment.
Removed the i24 test changes from the tree
There was a problem hiding this comment.
But now i24 is failing again
There was a problem hiding this comment.
I'm looking into it, I think it's not relevant for this PR, you can merge it anyway
- Prevents issue with RunEngine event_loop not being available - Reduces duplication - Expandable to new beamlines
DiamondJoseph
force-pushed
the
beamline-creation-readme
branch
from
a258c40
to
217cc52
Compare
Addresses #321 (hopefully in time to assist creation of i20-1 beamline, help get i22 merged)
Instructions to reviewer on how to test:
Checks for reviewer