Skip to content
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

TangoShutter: refactor (changes from https://github.com/mxcube/mxcubecore/pull/847/) #1158

Merged

Conversation

mockoocy
Copy link
Contributor

@mockoocy mockoocy commented Feb 20, 2025

Adds code changes from PR #847
And a test case for the TangoShutter Hardware Object.

At MAXIV we use these changes for quite a long time, but it has not been merged in the original for some reason (which is rather unclear looking at the PR's discussion).
The difference between the version at #847 and this PR is addition of type annotations and adoption of new linting rules.

@mockoocy mockoocy marked this pull request as draft February 20, 2025 08:07
Originally added in github.com/mxcube/pull/847
@mockoocy mockoocy force-pushed the pm-add-tango-shutter-changes branch from 113fe3a to 26e1677 Compare February 20, 2025 08:17
@mockoocy mockoocy force-pushed the pm-add-tango-shutter-changes branch from 26e1677 to acb0962 Compare February 20, 2025 08:43
@mockoocy mockoocy marked this pull request as ready for review February 20, 2025 08:55
The ruff config had to be adjusted to ignore the rule saying that
methods have to start with lowercase letter - as there is a mocked
Tango Device.
@mockoocy mockoocy force-pushed the pm-add-tango-shutter-changes branch from acb0962 to afb38fe Compare February 20, 2025 13:25
self.config_values = None

def _update_value(self, value):
def _update_value(self, value: DevState) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure the value is DevState type. I believe this is the output of the DevState command, which is a str.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I checked the type of value there during runtime I got a DevState type.
I believe that the state_channel could be set to return a string (with TangoChannel.read_as_str set to True).
I think setting it as DevState | str is a safe choice then :)

@beteva
Copy link
Member

beteva commented Feb 24, 2025

Nice with the test

@fabcor-maxiv
Copy link
Contributor

I recommend you check how the docstrings are rendered:

https://mxcubecore--1158.org.readthedocs.build/en/1158/dev/autosummary/mxcubecore.HardwareObjects.TangoShutter.html#module-mxcubecore.HardwareObjects.TangoShutter

@marcus-oscarsson
Copy link
Member

Oh yes, the documentation looks a bit strange.

@marcus-oscarsson
Copy link
Member

Thanks, alot. I think its really worth taking a look at how the documentation is rendered, there seems to be a tag missing or something similar.

beteva and others added 2 commits February 24, 2025 19:38
Added code-block to improve documentation.
Also changed type hint for `TangoShutter._update_value`.
The value could possibly be a string (i.e. using `TangoChannel` with
`read_as_str` set to True).
@mockoocy mockoocy force-pushed the pm-add-tango-shutter-changes branch from 167fd15 to 51a65d0 Compare February 25, 2025 10:12
@mockoocy
Copy link
Contributor Author

Thanks @beteva for fixing the xml code rendering in the docs! :)

I also pushed some changes, information about return rendered wrongly, when there was no space between docstring summary and Args:, Returns:, etc. blocks. (:returns text appeared in the summary)

@fabcor-maxiv
Copy link
Contributor

Looks like there is still some issue on the module docstring here:
image

@mockoocy mockoocy force-pushed the pm-add-tango-shutter-changes branch from c9f788b to 79a42b6 Compare February 25, 2025 11:09
The exceptions for capital case functions (Tango commands) are moved to
the functions that offend them instead of creating an exception for the
whole file.
@marcus-oscarsson marcus-oscarsson merged commit 8221e93 into mxcube:develop Feb 26, 2025
6 checks passed
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.

5 participants