mxcube / mxcubecore

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

[Documentation Code Camp] docstrings for emitted signals #866

Open meguiraun opened 3 months ago

meguiraun commented 3 months ago

Initial work for documenting emitted signals. I initially focused on the abstract classes (not all done yet 🤪)

Example output:

image
fabcor-maxiv commented 3 months ago

Wouldn't it make more sense to document the emitted signals at the class level rather than at the method level?

If I use an actuator I want to know what signals it emits and when (under what circumstances, i.e. what is the meaning of the signal), but I do not need to know which of the actuator's methods actually emits the signal, do I?

And same thing, if I want to implement a new (concrete) actuator, I want to know what signals my actuator should emit, but I will choose which methods will actually emit those signals.

cc @elmjag

beteva commented 3 months ago

@meguiraun thanks for the work. Good start. There is one slight problem - I think AbstractAperture is an obsolete class, There will be very soon a new PR to handle aperture test and mock up, but they inherit from AbstractNState. In this context, would you mind if we see together which are the obsolete abstract classes. @fabcor-maxiv I guess it is all right if we document the signals on the class level only. Actually this is the case right now, but certainly needs a bit of explanation which signal means what.

Maybe we should make a general signals documentation file, which lists all the "standard" signals, like valueChanged, limitsChanges, stateChanged, specificStateChanged, as well as the methods defined in BaseHardwareObjects.py - the emit method (namely its signature in order to explain how to emit a signal) and the force_emit_signals method, available for overloading . If we go for this, than we could explain the usage of these signals and in the classes only mention which are the emitted signals, and only document if there are other signals, specific for the class. What do you think?

rhfogh commented 3 months ago

I agree with @beteva - The biggest need is what are the general kinds of signals that are used to tie the application together, where they come from, and what they do. In a way the biggest question is not so much 'which signals does this class emit' but 'which signals should I listen for' and 'which signals do I need to emit in my own class?'.

meguiraun commented 3 months ago

okey, I see your point. I will leave the methods out.

yes, totally agree with a general documentation page (why, where in code are coming, some basic example, etc.), I had it in mind but I thought to start with documenting the code

meguiraun commented 3 months ago
  1. Added a new md with signal documentation, very early, please @beteva @rhfogh have a look, and if you feel so commit any improvement
  2. Updated, let me know if the "new" AbstracActuator class documentation is ok:
image
fabcor-maxiv commented 3 months ago

In many of the modified docstrings, it feels like adding the correct type hints would be nearly as efficient (especially for the return values).

rhfogh commented 3 months ago

@fabcor-maxiv HardwareObjectMixin woudl be the right choice throughout. HardwareObjects inherit from either HardwareObject or HardwareObjectYaml, but both inherit from HardwareObjectMixin,.

meguiraun commented 3 months ago

preview in https://mxcubecore--866.org.readthedocs.build/en/866/dev/hwobj_signals.html

beteva commented 3 months ago

Very good job @meguiraun 👍 The device and the equipment signals are definitely deprecated. Quite some time ago there was a decision not to use Device and Equipment class any more. You are right, we should remove the update signal and only use valueChanged. You can also remove the xrf related signals - in my WIP for xrf I've changed them all to stateChanged

marcus-oscarsson commented 3 months ago

Well done ! very nice !. As Antonia already pointed out, we should really start to remove the Device and Equipment classes.

meguiraun commented 3 months ago

while the signals are still there, I simply marked as deprecated. Also added queue and diffractometer

meguiraun commented 3 months ago

AbstractBeam with docstrings in class here AbstracActuator with docstrings outside class here

I can remove all changes to the abstract classes if it makes the other PR easier to handle

ping @beteva

fabcor-maxiv commented 3 months ago

AbstractBeam with docstrings in class here AbstracActuator with docstrings outside class here

For me, this belongs in the class docstring : )

meguiraun commented 3 months ago

AbstractBeam with docstrings in class here AbstracActuator with docstrings outside class here

For me, this belongs in the class docstring : )

I think same way, it seems its place

beteva commented 3 months ago

@meguiraun, agree the best place is the class docstrings.

marcus-oscarsson commented 2 weeks ago

Hey @meguiraun, do you have an update on this one, it seemed almost done ?