Skip to content

Use Reference to prevent Device name mangling of undulator and dcm#957

Merged
shihab-dls merged 3 commits intomainfrom
926_undulator_dcm_should_use_reference
Dec 16, 2024
Merged

Use Reference to prevent Device name mangling of undulator and dcm#957
shihab-dls merged 3 commits intomainfrom
926_undulator_dcm_should_use_reference

Conversation

@shihab-dls
Copy link
Contributor

Fixes #929

Instructions to reviewer on how to test:

  1. Check for correct usage of Reference

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}

@shihab-dls shihab-dls requested a review from a team as a code owner December 12, 2024 12:22
@codecov
Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.38%. Comparing base (45eb5e0) to head (88be43f).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #957      +/-   ##
==========================================
+ Coverage   95.80%   97.38%   +1.57%     
==========================================
  Files         144      143       -1     
  Lines        6179     6079     -100     
==========================================
  Hits         5920     5920              
+ Misses        259      159     -100     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rtuck99
Copy link
Contributor

rtuck99 commented Dec 13, 2024

Thanks for this Shihab!
The code changes here look fine, however before merging this, you will need to make changes and raise another PR in mx-bluesky, because the changes here are exposed to calling code in mx-bluesky, and this PR as it stands can't currently be merged without breaking it.

If you run the unit tests and search around in mx-bluesky you will find that there are several places where there are similar dereferences of undulator_dcm.dcm/undulator_dcm.undulator.

You might also want to alter the name of these to make it clear that they are reference wrappers rather than the actual devices - elsewhere where I have added them I have suffixed the names with _ref to make this clear.

A second consideration is whether to make them private by prefixing an underscore, however I think in this case it's ok to leave them public as it's probably both expected and useful to expose the undulator and DCM devices that are backing this composite.

Copy link
Contributor

@rtuck99 rtuck99 left a comment

Choose a reason for hiding this comment

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

See previous comment

@shihab-dls
Copy link
Contributor Author

Thanks Rob! I've opened up a PR in mx-bluesky amending the exposed references, so I'll tag you and Ollie as reviewers on that one.

@shihab-dls shihab-dls requested a review from rtuck99 December 16, 2024 09:41
@shihab-dls shihab-dls changed the title Using Reference to prevent Device name mangling of undulator and dcm Use Reference to prevent Device name mangling of undulator and dcm Dec 16, 2024
Copy link
Contributor

@olliesilvester olliesilvester left a comment

Choose a reason for hiding this comment

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

Looks good!

@olliesilvester olliesilvester dismissed rtuck99’s stale review December 16, 2024 12:18

Changes have been addressed

@shihab-dls shihab-dls merged commit 275314a into main Dec 16, 2024
@shihab-dls shihab-dls deleted the 926_undulator_dcm_should_use_reference branch December 16, 2024 16:58
huw-dls pushed a commit that referenced this pull request Dec 18, 2024
)

* Using Reference to prevent Device name mangling of undulator and dcm

* suffixing undulator and dcm references with _ref

* Move super().__init__ to end of constructor

---------

Co-authored-by: Shihab Suliman <rye74444@ws557.diamond.ac.uk>
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.

Use references in undulator_dcm

3 participants