Code quality improvements to the TangoShutter Class #847
Conversation
Co-authored-by: fabcor <[email protected]>
This reverts commit 557ff3c. Going back to pre - git hooks.
…mport gevent, changing import order
Updating local repo
Update pre commit
Co-authored-by: fabcor <[email protected]>
Merging the latest TangoShutter changes : adding logging and some coments
|
Just pushed the latest updates. I added logging as well. |
|
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 ("). :) |
|
Just updated the comment section with the example provided by @beteva as well as a warning about using only double quotes. |
There was a problem hiding this comment.
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
- 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
- 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.
|
@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 " |
|
As a side effect for testing this code, we wrote some unit tests for this implementation of TangoShutter: Fell free to include this into this PR. If that complicates thing to much, I'll make a separate PR in the future. |
|
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. |
|
Oh! But I see now, that this is already done with mxcubecore/mxcubecore/HardwareObjects/abstract/AbstractNState.py Lines 80 to 88 in e4b2434 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 |
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Import all changes from this PR: mxcube#847 TODO: drop this commit once the PR have been merged, or if rejected, deal with it
Adds code changes from PR mxcube#847 And a test case for the TangoShutter Hardware Object.
Also adds a test case for it. Originally added in mxcube#847 with some additional changes (i.e. type hints).
Originally added in github.com/mxcube/pull/847
Originally added in github.com//pull/847
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.