libreant icon indicating copy to clipboard operation
libreant copied to clipboard

libreant-manage preset_check

Open ael-code opened this issue 10 years ago • 8 comments

I would like to implement an utility to check only the presets from cli without launching webant. But with the current libreant-manage architecture this is impossible. I need to launch PresetManager stand-alone with strict=True but LibreantCoreApp already launches it.

ael-code avatar Feb 21 '15 10:02 ael-code

On Sat, Feb 21, 2015 at 02:13:38AM -0800, ael wrote:

I need to launch PresetManager stand-alone with strict=True but LibreantCoreApp already launches it.

just set an option PRESET_STRICT which LibreantCoreApp will follow. Then libreant-manage can set that option as default.

boyska avatar Feb 21 '15 10:02 boyska

just set an option PRESET_STRICT which LibreantCoreApp will follow. Then libreant-manage can set that option as default.

I did not understand

ael-code avatar Feb 21 '15 10:02 ael-code

In LibreantCoreApp, instead of

        self.presetManager = PresetManager(self.config['PRESET_PATHS'])

we can do

        self.presetManager = PresetManager(self.config['PRESET_PATHS'], self.config['PRESET_STRICT'])

At that point, inside libreant-manage, we can force that setting to be true.

Oh, and app.presetManager should not be set at initialization time, but upon actual usage, just like we do for get_db.

boyska avatar Feb 21 '15 11:02 boyska

self.presetManager = PresetManager(self.config['PRESET_PATHS'], self.config['PRESET_STRICT'])

this do not solve my problem. I want to run presetManager stand-alone inside a cli command

ael-code avatar Feb 21 '15 11:02 ael-code

then just run it. But I still believe that this approach is slightly flawed. If you want to to a check on a skeleton, for example, why don't you run self.presetManager.check_skeleton_validity(filename) (I'm inventing this method call, but you probably will get the point)?

Changing the initialization option, you are changing the way resources are defined. But a presetcheck should not change how the resources are defined. It's just about asking details about validation errors.

The problem, for how I see the new code, is that error check produce either a logging or an exception. It doesn't seem to me that there is a way to get the errors, which is what a presetcheck actually need, apart from catching exception.

I also cannot understand why loading a preset require a whole instance of a PresetManager: shouldn't the "load_preset" method become a standalone function? It would make this kind of things easier.

boyska avatar Feb 21 '15 13:02 boyska

then just run it. But I still believe that this approach is slightly flawed. If you want to to a check on a skeleton, for example, why don't you run self.presetManager.check_skeleton_validity(filename) (I'm inventing this method call, but you probably will get the point)?

Changing the initialization option, you are changing the way resources are defined. But a presetcheck should not change how the resources are defined. It's just about asking details about validation errors.

It is very simple: if you want to validate a single skeleton file or multiple skeleton file you can call PresetManger(paths); if you have a skeleton as python object you can use Preset(pyObj) to validate it.

The problem, for how I see the new code, is that error check produce either a logging or an exception. It doesn't seem to me that there is a way to get the errors, which is what a presetcheck actually need, apart from catching exception.

The exception/logging messages are very explicative. Please reproduce some errors if you have not yet done.

I also cannot understand why loading a preset require a whole instance of a PresetManager: shouldn't the "load_preset" method become a standalone function? It would make this kind of things easier.

PresetManager is a wrapper around load_preset, it's doing some path controls, so you will not call load_preset directly.

ael-code avatar Feb 22 '15 11:02 ael-code

The problem, for how I see the I also cannot understand why loading a preset require a whole instance of a PresetManager: shouldn't the "load_preset" method become a standalone function? It would make this kind of things easier.

PresetManager is a wrapper around load_preset, it's doing some path controls, so you will not call load_preset directly.

Which is exactly the problem. Let's get back to the original problem: you need to validate presets. If you are able to call a load_preset method, you can just do this. The problem here, is that you cannot. That's the same reason why load_preset is badly covered by unittests, I suppose.

Just make code more modular, and I guess you'll be fine.

Some practical suggestions:

  • make inner code accept buffer, not filenames. This allows for easier unittesting with stringio, and is much more general.
  • clearly state what every class or method is doing in the docstring. At the moment, I am not able to summarize the goal of load_preset, apart from generic thing like "it loads a preset". I cannot understand what depth does, also. What is a Schema? what concept does it represent? how does it behave? Providing a glossary and using consistent naming would also be very useful.

boyska avatar Feb 22 '15 12:02 boyska

there is no libreant-manage anymore. The current style is having specialized libreant-* command, which would fit your usecase better.

So if you want to implement this, it can be done with the current design.

boyska avatar Nov 03 '15 11:11 boyska