search

mxcube / mxcubecore

Backend used by MXCuBE
http://mxcube.github.io/mxcube/
GNU Lesser General Public License v3.0
11 stars 52 forks source link

Update base classes with type hinting and add test cases #746

Closed oldfielj-ansto closed 1 year ago

oldfielj-ansto commented 1 year ago

This PR adds type hinting and low level hardware object test cases.

Currently the goal is to add type hinting and test cases for each class defined in the modules BaseHardwareObjects.py and BaseHardwareObjects.py, which has not yet been deprecated.

Once these test cases are completed it will be a lot simpler to create test cases for the abstract hardware objects, as all the low level logic can be patched over using MagicMocks.

oldfielj-ansto commented 1 year ago

Still a work in progress, not ready to merge.

marcus-oscarsson commented 1 year ago

Looks really nice !. I'm not entirely up to speed with typhints so maybe there is something I did not understand with how Union[str, Any] is used. Would not simply Any be enough ?. Or is it more to signal that it should be str but its in reality in many cases not a str and so Union[str, Any] is partly used for documentation ?

marcus-oscarsson commented 1 year ago

Hey @oldfielj-ansto, do you think you can rebase this when you get the time to solve the conflicts, then Ill merge. Thanks !

oldfielj-ansto commented 1 year ago

This is very, very good. As yuou see I have some suggestions/questions, but I would not insist on holding this up - we can always come back to them.

In a way it is too bad to have done so much work on (e.g.) HardwareObjectNode when,. (IMHO) it ought to be deprecated. But it is great to have a start on introducing type annotations.

Thanks, I still think it's worth doing it while these objects aren't fully deprecated, even then, they may be around for a while yet for legacy compatibility reasons while facilities update their codebases.

oldfielj-ansto commented 1 year ago

Looks really nice !. I'm not entirely up to speed with typhints so maybe there is something I did not understand with how Union[str, Any] is used. Would not simply Any be enough ?. Or is it more to signal that it should be str but its in reality in many cases not a str and so Union[str, Any] is partly used for documentation ?

Thanks, the section @rhfogh commented on has been updated to simply be typed as "str". As mentioned above, I mostly just did this to indicate that I was fairly sure a string was the expected type, but wasn't entirely sure, so left the typing open.

oldfielj-ansto commented 1 year ago

Hey @oldfielj-ansto, do you think you can rebase this when you get the time to solve the conflicts, then Ill merge. Thanks !

Sorry @marcus-oscarsson I've been pretty busy with other parts of the MX3 implementation, haven't had a chance till now to follow up on this.

I've tidied things up and rebased my changes onto the latest commit.

marcus-oscarsson commented 1 year ago

Great !, thanks for all this nice work !