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.

[rhfogh]

I am sorry, but I really, really hate this. I can see that this particular PR is not making the difference; the main point is already merged in, so it is too late. But really:

Putting JSON strings inside XML is bad in itself. But worse, we have here a major change to the configuration system, that has not been discussed as such, and that is only valid for TangoShutters and only implemented inside TangoShutter.py. How are we to keep track of what is happening, if every class uses its own custom configuration system? What happens when someone has to make a new class and looks for examples on how to configure it? What happens if someone else reads inside those XML properties (you are allowed to) and tries to untangle this on his own, maybe to figure out (from the configuration files) what the applicable states for a TangoShutter are?

Supposedly, you can store any data structure you need with XML. It is not very practical, which is why moving to another format seems such a good idea, but while we are agreed on using XML surely the correct procedure is to take the trouble and use XML, not to put in your own hacks.

What I would like to see at this point is either

1. a proposal for how we are to use JSON-inside-XML as the overall configuration system for MXCuBE in the future, complete with rules for how it is applied and a rationale for why it is a good idea.

or

2. An explanation for why this is a temporary aberration, and how we can make sure that it will go away again without serving as a pattern for future implementations.

[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:

elmjag@6076956

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:

mxcubecore/mxcubecore/HardwareObjects/abstract/AbstractNState.py

Lines 80 to 88 in e4b2434

	def initialise_values(self):
	"""Initialise the ValueEnum with the values from the config."""
	try:
	values = ast.literal_eval(self.get_property("values"))
	values_dict = dict(**{item.name: item.value for item in self.VALUES})
	values_dict.update(values)
	self.VALUES = Enum("ValueEnum", values_dict)
	except (ValueError, TypeError):
	pass

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?

[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/mxcubecore@6076956 . Thanks @fabcor-maxiv

[elmjag]

Please rename this variable to for example VALUES_LITERAL. The VALUES_JSON is now confusing, as technically speaking we are not supporting JSON strings but pythong literal expressions. Event if it so happens in this case that text is identical.

[beteva]

Hi @elmjag, I am a bit puzzled that we need a special test for the TangoShutter and not using the generic shutter test. Do we miss something in the generic shutter test or the ShutterMockup. If such is the case, maybe it is worth adding your work to the generic shutter test, so it would serve other shutter types, not just the Tango ones.

[elmjag]

These are tests to test code in TangoShutter.py. The generic shutter test code does not cover it, as it is using ShutterMockup hardware object.

More specifically these tests set-up an actual tango device and instantiate a TangoShutter hardware object that talks to that device. This way you check that opening/closing shutter work, with TangoShutter hwo, using real tango stack.

They way I read the code in test/pytest/test_shutter.py, it is testing the code in the abstract classes starting with AbstractShutter.

The tests in test_shutter.py and test_hwo_tango_shutter.py are testing related, but separate aspects of the code base.

[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 ?