Code quality improvements to the TangoShutter Class

[HilbertCorsair]

A more formal way to retreve unconventional shutter states from the xml configuration file and add them to the class standard values dictionary with conventional names.

[HilbertCorsair]

Just pushed the latest updates. I added logging as well.

[HilbertCorsair]

I don't know how relevant this is but, if somene needs to use a list in the xml config file , make sure to use double quotes ("). I copy-pasted the configuration from the comment to the xml file for a test and it had replaced the double quotes (") with simple quotes (') resulting in a json parsing error. No (') are allowed in the json inside the xml ! Only ("). :)

[HilbertCorsair]

Just updated the comment section with the example provided by @beteva as well as a warning about using only double quotes.

[fabcor-maxiv]

@HilbertCorsair, what was the issue with XML here again? If this can be expressed with XML, this should be expressed with XML. I looked again at the previous pull request, but could not find the rationale why this does not use XML all the way. Did I miss it? Is there a reasoning written down somewhere?

[It might be my fault that we focused on the "eval vs. JSON" topic, instead of focusing on the "why not XML?", and I am sorry for that.]

[elmjag]

As a side effect for testing this code, we wrote some unit tests for this implementation of TangoShutter:

https://github.com/elmjag/mxcubecore/commit/60769564260604be18d51a0dede9b61ba5ecbacf

Fell free to include this into this PR. If that complicates thing to much, I'll make a separate PR in the future.

[HilbertCorsair]

I don't like the current configuartion solution either. I'm allready looking into configuration by yaml but this will take some time. I can try a different configuration using only XML. I will test this Monday. Bottom line is that we need a way to mapp any unconventional local tango shutter value : state pare to the conventional states definded in the class. I's not always necesarry and the code doesn't change anything in the configuration of Tango shutters that have conventional values. This is why the values tag in the xml file is optional, as a solution for any shuters with unconventional local tango values (CLOSE instead of CLOSED for example). I don't think there is an issue using just XML for this however if we want to have a general Tango shutter class I don't see an easy way arround have to use 2 configuration sysems (one for shutters with conventional values and one for shutters with unconventional values). The current version does this by importing the json dictionary whenever a shutter has nonconvetional values. I saw this in an example somwhere but I guess we don't neceserly need json.

[fabcor-maxiv]

Oh! But I see now, that this is already done with ast.literal_eval in one of the parent classes:

https://github.com/mxcube/mxcubecore/blob/e4b2434d1f2aa98dfba3ff10463ed89bebd5795e/mxcubecore/HardwareObjects/abstract/AbstractNState.py#L80-L88

Then we should definitely have stuck to this!

Can we see concrete examples of what we want to write in the XML file that can not be handled by AbstractNState.py already?

[marcus-oscarsson]

@fabcor-maxiv, @HilbertCorsair, In the end I merged the PR so Im perhaps the one that is responsible for not being more vigilant. The discussion around eval was good an welcome, so nothing to worry about there. @rhfogh Is also completely right that we should definitely avoid sidestepping the current configuration system more than we are already doing with ast.literal_eval. Which in itself is not a XML construct and something we have done because we thought it was convenient.

@fabcor-maxiv also points out something important, what does json.loads solve that can't be done with whats already implemented in the base class AbstractNState ?. If the answer is very little I'd say that is worth using whats implemented there and maybe make the necessary changes in TangoShutter.

As we said during our last developers meeting, we should now come up with a plan how to migrate from XML to YAML. Ill create an issue for this within shortly where we can discuss how to best proceed.

[HilbertCorsair]

I need some advice here before submiting further modifications to this TangoShutter class. As pointed out by @fabcor-maxiv the current configuration for the shuter class uses a json dictionary inside the values tag of the XML configuration file. This can be evidenced in mxcubeweb/tast/HardwareObkectsMockup.xml/safety_shutter.xml and it's not something I came up with @rhfogh . I can change the TangoShutter class to take a different configuration based on pure XML, something like this :

<object class="ShutterMockup">
    <actuator_name>safety_shutter</actuator_name>
    <username>SafetyShutter</username>
    <values>
        <value state="OPEN">open</value>
        <value state="CLOSED">closed</value>
    </values>
</object>

This removes the need for a json dictionary but will probably break the initialize_values() method of the AbstractNclass and I think this would be a more significant change to the configuration. If we opt for this solution I will have to refactor both the AbstractNState and the TangoShutter class. Another aspect I'm not sure I understand is : why do we need to account for lists in the values of the configuration file? If my understanding is correct we shouldn't need to use lists in the configuration unless some tango shutter somewhere has a list as specific state. Is this the case? This could be configuerd in XML in the following way :

<object class="ShutterMockup">
    <actuator_name>safety_shutter</actuator_name>
    <username>SafetyShutter</username>
    <values>
        <value state="OPEN">open</value>
        <value state="CLOSED">
            <item>"MYSTATE"</item>
            <item>"BUSY"</item>
        </value>
    </values>
</object>

Another simpler temporary option would be to live the configuration and AbstractNstate as tehy are and just change the Tango shutter class to use values = ast.literal_eval(self.get_property("values")) as in the AbstractNstate. This would avoid using json but I wouldn't say it's a bretter solution. How should I move foreward with this? Do I refactor the AbstractNstate and TangoShutter to remove the json from the configutation? (shouldn't be too hard or impactfull as a change) do I only change the TangoShutter class to use literal_eval instead of json? Living things as they are for now is also an option. Any recomandations? @marcus-oscarsson @rhfogh

[fabcor-maxiv]

If one of the parent classes already uses ast.literal_eval() (AbstractNState), then my recommendation is to also use this in the TangoShutter class (whether the Tango shutter can use the parent method as-is or if it needs to be overridden with some tweaks, I have not understood).

[HilbertCorsair]

@fabcor-maxiv Done!

[fabcor-maxiv]

@HilbertCorsair Thanks for your work :)

We plan on testing this latest version tomorrow.

Have you considered adding the basic unit test to this PR?

[HilbertCorsair]

I just added the basic unit test proposed by @elmjag : elmjag@6076956 . Thanks @fabcor-maxiv

[elmjag]

I have tested this PR, it works fine!

Just one addition nitpick comment. Can we please squash all commits in this PR? 20+ commits feels a bit, ahum, noisy in the git history.

[beteva]

Thank you everybody for the efforts to make the perfect TangoShutter Hardware Object. I think this has been a very useful not only for the TangoShutter, bur also to serve as a model not only for similar hardware objects, which would inherit from AbstractNState, but also for hardware objects, using Tango commands and properties. Being one of the authors of the AbstractNState, I need to explain what is the idea behind _initialise_values and ast,literal_eval. The VALUES enum is the one, used by any GUI to set/get the value of the HardwareObject. In the case of NStates, we expect the particular values, not defined in the AbstractNState class to be defined in the configuration file as a dictionary, with keys being the new name for the enum member and values, which can be of type str, bool, int, float, bur also tuple. We than convert this dictionary to an eval and add it to the already defined members. The dictionary can contain not only new members of the eval, but also redefine the value of already existing ones. For this we use ast.literal_eval and defined the _initialise_values method To make this more clear, here are some real case examples: <values>{"IN": "BEAM", "OUT": "OFF"}</values> - beamstop <values>{"IN": False, "OUT": True}</values> - front and back light in/out <values>{"A30": (1, 30, 0.94), "A15": (2, 15, 0.78)}</values> - aperture (index,size,factor) We've chosen a tuple because of the conversion to enum, as VALUES is an enum. Maybe this will make more clear the implementation for the TangoShutter and its parent class.

[marcus-oscarsson]

I'm happy to merge this but there is a conflict, could you have a look @HilbertCorsair ?