Skip to content

Conversation

@Relm-Arrowny
Copy link
Contributor

Fixes #ISSUE

Instructions to reviewer on how to test:

  1. Do thing x
  2. Confirm thing y happens

Checks for reviewer

  • Would the PR title make sense to a scientist on a set of release notes
  • If a new device has been added does it follow the standards
  • If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
  • Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

@Relm-Arrowny Relm-Arrowny linked an issue Jan 26, 2026 that may be closed by this pull request
17 tasks
@codecov
Copy link

codecov bot commented Jan 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.09%. Comparing base (a7dde93) to head (168f0a2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1861      +/-   ##
==========================================
+ Coverage   99.07%   99.09%   +0.01%     
==========================================
  Files         297      317      +20     
  Lines       11298    11480     +182     
==========================================
+ Hits        11194    11376     +182     
  Misses        104      104              

☔ View full report in Codecov by Sentry.
📢 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.

@Relm-Arrowny Relm-Arrowny mentioned this pull request Jan 26, 2026
17 tasks
Copy link
Contributor

@oliwenmandiamond oliwenmandiamond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to set logging and beamline for every single device file.

I also really dislike needing to define PREFIX 20 times for each device file. However, we also cannot add it into i10.py and then import as then we would have a circular import

The only solution that I can think of is we define

i10.py
i10_prefix.py

and then have i10 and i10/devices/my_device.py import from i10_prefix.py

I also don't think this is layed out correctly.

You should have i10.py, i10_1.py and i10_shared.py

Anything that is shared between endstation lives in shared, e.g the insertion device, shared diagnostics, shared mirrors etc.That way when looking at a beamline, it is very clear what is specific to this end station, e.g i10? Everything defined in i10.py. What is shared between end stations? Everything in i10_shared. It will make supporting and maintaining much easier.

@oliwenmandiamond
Copy link
Contributor

I think maybe a solution to this would be to define prefix as devices.fixture inside i10.py

e.g

def prefix() -> BeamLinePrefix:
    bl = get_beamline_name("iXX")
    return BeamlinePrefix(bl, suffix=suffix)

then have every device expect prefix: BeamlinePrefix as an dependency. This means then that running dodal/devices/i10/devices/my_device would fail but from i10.py it would be fine which I think is okay. However, not sure it is worth the boiler plate to separate the files. I think separating into shared would be enough and maybe a large group of devices like insertion devices is worth it.

@Relm-Arrowny
Copy link
Contributor Author

@oliwenmandiamond I don't disagree, I was just thinking about grouping devices so share was a after thought, I will fix them if we decide to go down this route. it was mostly copy and paste hence all the PREFIX and helper functions, if we going down this route, II think we can dorp the helper functions all together, as often we are just defining everything for a single use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

YAML Configuration Generation

3 participants