Open stanislaw47 opened 4 years ago
Hi @stanislaw47 Thanks a lot for this contribution!, here are a few preliminary comments:
getLoader
is called, and then the returner loaded used, right?getLoader
should be implemented with entry-points. I think it is relatively easy (I've been doing some work in preparation of TEP13 lately and I can help with it. I suggest to use the following entry point name: taurus.gui.loaders
and iterate over it in getLoader
. Also, the *ConfigLoader
itself can declare its supported extension(s) (and we treat the pyconf case as an special fallback case)TaurusGui.loadConfiguration()
will also need to be refactored to use plugins, but this can be done in two steps.Hi @cpascual
I really like the idea, and I am happy because it comes to clean quite a messy part of Taurus (the config loading)
I'm happy to hear it
Unless I missed something (I gave a quick glance only) , I thing this PR still misses the part in which the getLoader is called, and then the returner loaded used, right?
Actually, it's there, in here. Resulting conf
object is instance of *ConfigLoader
nad is passed to specific loading methods.
I agree in that getLoader should be implemented with entry-points. I think it is relatively easy (I've been doing some work in preparation of TEP13 lately and I can help with it. I suggest to use the following entry point name: taurus.gui.loaders and iterate over it in getLoader. Also, the *ConfigLoader itself can declare its supported extension(s) (and we treat the pyconf case as an special fallback case)
I got trouble getting my head around pyconf since it support noto only extension but also directories and non-exisitng paths. But if it'll be treated as special fallback case, then I would be trivial indeed. I'll try to implement it soon.
As for the abstract loader: I think this is a good opportunity to clean old/deprecated stuff. For example, the monitor stuff can be removed. Also, the sardana-related configs should not be implemented/required in taurus. A possible solution is to allow multiple config loaders for the same extension (e.g. if you provide a json file, all the loaders that declare themselves supporting json would be activated and used, and in this way taurus would declare a json loader that handles onlyy taurus configs while sardana would declare a json loader that handles the sardana-related configs, and both would be used). Note that in order to completely isolate taurus and sardana configuration handling, the TaurusGui.loadConfiguration() will also need to be refactored to use plugins, but this can be done in two steps.
I am happy to remove deprecated stuff like MONITOR
or CONSOLE
, I kept them for backward compatibility. About multiple loaders for same file extensions - I actually do not know how that would work with entrypoints. Would one plugin override the other? Or would it allow them to work at the same time? Could you describe it a little more?
Regarding python2 compatibility: I do not expect to maintain py2 support in taurus after the Jul20 release, so I personally would not struggle with making the new feature py2-compatible. If it is just a matter of the small compatibility hack with abc, then it is fine, but if it requires more work, I would make this a py3-only feature and fall back to the old mechanism for py2 if we integrate this PR before Jul20. The same for Qt4.
For now, only compatibility is indeed ABC
hack so I'll just keep it compatible for both Python 2 and 3. I believe this PR does not require special care for PyQt version, at least for now.
Regarding tests: I just did a large refactoring of the tests in #1114 . As soon as it is merged, you should update this branch with it. Then I would use pytest to do unit tests for the loaders and for the backwards-compatibility
Sure think, will do. I've written some tests using pytest so that should not be a problem.
Do you have any plans for a "*ConfigWriter?". At the moment, both the "new gui wizard" and the TaurusGui themselves are able to create xml config files.
Dumping configuration in given format would be cool feature. However, I think that might be non-trivial for py files. And could make this PR much bigger. Regarding plugins - should they support both loading and dumping configuration? Or just loading should be mandatory and dumping a feature? I personally would go for both loading and dumping as mandatory.
About multiple loaders for same file extensions - I actually do not know how that would work with entrypoints. Would one plugin override the other? Or would it allow them to work at the same time? Could you describe it a little more?
What I had in mind is: the taurus abstract loader only forces the strictly-taurus configs. Then sardana would define a similar abstract loader which provides the same interface but forces a complementary set of configs.
Now, anyone (taurus or a third-party) can register any number of ConfigLoaders. Each ConfigLoader should provide a .supports()
method which accepts the config source id (name of the config file/dir/module) and returns True/False depending if it supports it (in this way we do not need to rely just on extensions, and pyconf gets supported just as any other loader).
When getLoader
is called, instead of returning a single *ConfigLoader
, it returns the list of those for which support(id)
returned True
. The list can be sorted for precedence in case of conflict (see note 1)
Then when loading the configs, all the ConfigLoaders would be called in order and allowed to update a config dict.
By the way: this method would eventually allow mixing config sources (e.g. it would be easy to naturally emulate the current situation in which configs are simultaneously loaded from a module and from an xml file)
Note 1: we can use the same mechanism as for the TaurusForm Item factory plugins to set the sort order
Note 2: (just a wild idea) maybe the ConfigLoader objects could present a dictionary-like API: this would make them quite natural to handle for operations such as "updating". It also would facilitate mocking for tests.
I've initially implemented plugable loader by using file extensions and pyconf as a special fallback (not pushed yet). However, using supports
method would make it easier. One disadvantage I can see is that it makes each loader actually loaded from entrypoint. That would (probably) make startup slower, however I do not know if it would be significant for users.
Using supports
would also make loading pyconf a bit more readable so obvious benefit here.
Then when loading the configs, all the ConfigLoaders would be called in order and allowed to update a config dict.
That would require a bit more refactoring. Now, configuration data is stored in properties of concrete ConfigLoader
. There's no notion of 'config dict'. Using multiple config loaders as it is now would override one another completely. And if some option is not present in second one, default value provided by TaurusGui.loadConfiguration
method would override value from first one.
So basically:
There would be needed for one more object, something like Configuration
class. It could expose dict-like API. Each loader would just extract data from its source and pass it to Configuration
object.
I think it has to have good architecture since there's high chance for repeating logic and managing data from wrong end.
By the way: this method would eventually allow mixing config sources (e.g. it would be easy to naturally emulate the current situation in which configs are simultaneously loaded from a module and from an xml file)
Hmm, not sure how it would exactly work with both Python module and XML file but I feel that it would be good way for supporting old configurations in backwards-compatible way.
using supports method would make it easier. One disadvantage I can see is that it makes each loader actually loaded from entrypoint. That would (probably) make startup slower, however I do not know if it would be significant for users.
Yes. I also thought about that, but the only alternative is using the registered plugin name for selection, and this would make it more difficult/ugly for allowing multiple loaders for the same type of source.
On the other hand, if we use the taurus.core.util.selectEntryPoints
API, we can mitigate this by pre-filtering the entry point candidates if needed.
There would be needed for one more object, something like Configuration class. It could expose dict-like API. Each loader would just extract data from its source and pass it to Configuration object.
Yes, this is more or less what I had in mind when I talked about "updating a config dict". I was imagining this Configuration object to be just a dict and then having the extra required logic in the loadConfiguration part, ... but probably using a Configuration class to encapsulate all this merge/update and load logic is cleaner.
Just another wild idea... maybe if we do things generic enought, we could leave the door open to also integrate the loading of .ini settings in this mechanism (not necessarily in this PR!)
I've decided to start small - now configuration is just passed around in dictionary. Each loader just creates dictionary with fields and values from configuration. Then, all created dictionaries are merged into one in order of loaded entrypoints. I believe now it is much more flexible for change and plugging in new loaders.
I've introduced field CONFIG_VALUES
(dunno if name is good enough) which is a list (to allow modifications, instead of tuple) of fields which can occur in configuration and be loaded. All those field are non-deprecated, Sardana-free, simple values. All *Description
like fields are loaded manually because I do not believe that they will require such flexibility anytime soon. Custom loaders for Sardana or for backward compatibility can extend it before loading stuff.
Another thing about new config values introduced by new loaders - they have to be loaded somehow. I think that there might be space for introducing hooks in TaurusGui.loadConfigurtation
method. Config loader could somehow register (list of) functions which would be executed during loadConfiguration
(probably at the end). Each call would pass TaurusGui
instance as first argument and conf
dictionary as a second argument. That would allow flexible mutation of TaurusGui
. I think it might be worth doing in this very PR.
note that #1114 is already merged, so if you merge develop into this branch you will already have the fixed testsuite
Hi @cpascual
sorry for the long break.
I've updated my branch with new approach to special config values (CONSOLE
i.e.). In general, there should be generic representation of config source( be it Python module, xml or json file) and yet another entrypoint (taurus.config.properties
). Functions registered using this entrypoint receive TaurusGUI
instance. This allows very flexible behavior nad loading extra stuff i.e. Sardana related.
I've put SYNOPTIC
into separate module because for me it seems like extra thing. I can revert it if this is necessary.
I would appreciate review to validate that this approach is good enough.
ok... give me a bit of time (I've my hands full adapting the testsuite so that it can be run with gitlab-ci). But do not hesitate to ping if I forget
Hello Taurus community This PR provides what I wanted in Taurus for long time: ability for load configuration for TaurusGUI from JSON.
I created abstraction for config source, be it a Python module or XML/JSON file. In that way, adding support for any other config file format, like TOML or YAML should be trivial. TaurusGUI now loads configuration by accessing fields of
*ConfigLoader
class. Whole interface of*ConfigLoader
class is defined inAbstractConfigLoader
by using Pythonabc
module.All discovery logic happens in
taurus.qt.qtgui.taurusgui.config_loader.getLoader
function. It matches loaders by file extensions. In case of directory or non-existing path, it tries to usePyConfigLoader
. In case of using entry-points, logic has to be moved elsewhere but for the moment I do not know how to achieve it, especially when taking into account possibility to pass directory and Python module path.Regarding
abc.ABC
class for Python 2 and 3 - I know that Taurus usesfuture
package for compatibility. However, I did not found anything there about providingabc.ABC
so I created my own compatibility from what I found on StackOverflow.Things to do: