Modified file loader for readability

[TeresiaOlsson]

I have modified the file loader to hopefully make it easier to understand since I forget between times when I looked at it. There is now a slightly different structure and more docstrings and comments to help remember how it works.

Biggest change is that I added a context manager to keep track of the stack of files. This I expect that normal users will likely not really understand (since I also forget and need to look up how context managers work every time) but I think it still helps to improve the readability. I also added some comments to help understand the steps it does.

I also changed the name use_fast_loader to include_locations to maybe make it easier to understand why there is a fast and slow loader and why you not always wants to go for the fast one. It still says in the docstring that including locations will make it slower.

I changed so the default is now to use the fast loader and the user needs to specify if they want to use the slow loader and include the location metadata.

[JeanLucPons]

As @gupichon is the author of this cycle detection I let him review.
Take care that there is no unit test for absolute path which was working.

[TeresiaOlsson]

Good point. I should also look at the unit tests and maybe add some more to check that the behaviour has actually not changed.

[JeanLucPons]

Thanks for clarifying this code, few comment lines can help a lot !

[TeresiaOlsson]

I also discovered now that there already is a path to load without adding location metadata which is what I needed to finish #279 where the factory no longer need to handle validation errors. I just need to use the fast loader for the case when the user wants to skip validation :)

[TeresiaOlsson]

I'm wondering if we should change the default behaviour and use the fast loader as default? Then the user normally gets the fast loader and only if they get an error and want more information to be able to debug they need to switch to the slow loader.

[JeanLucPons]

Yes this is a common workaround. You load using fast loader and when it fails you reload using slow loader. This can be done in the Accelerator class.

[TeresiaOlsson]

The PR to add the option to use environment variables solving issue #310 has now been merged into this PR.

[gupichon]

Many thanks for these developments @TeresiaOlsson! From my point of view, it's a great improvement.
However, I'd like to have a small test for environment variables like this one:

@pytest.mark.parametrize(
    ("install_test_package", "config_file"),
    [
        (
            {"name": "tango-pyaml", "path": "tests/dummy_cs/tango-pyaml"},
            "tests/config/env_var_config.yaml"
        )],
    indirect=["install_test_package"],
)
def test_environment_variable(install_test_package, config_file:str):
    test_var = "Environment variable test"
    with patch.dict(os.environ, {"TEST_VAR": test_var}):
        acc = Accelerator.load(config_file)
    assert acc.get_description()==test_var

With this file:

type: pyaml.accelerator
facility: ESRF
machine: sr
energy: 6e9
data_folder: /data/store
description: ${env:TEST_VAR}
devices: []

Regarding this specific tag (env), I'm wondering if it shouldn't be resolved by Pydantic somehow, so that it could be used either in a file or within a generically built dictionary. This way, the from_dict method would resolve it too. Maybe with a model validator like the ones used in PySC. However, this will be for later anyway.

[gupichon]

My bad, I've found the tests with environment variables.

[gupichon]

Great job!
The only missing feature would be the ability to use environment variables without configuration files, but that's truly not mandatory.

[TeresiaOlsson]

Great job! The only missing feature would be the ability to use environment variables without configuration files, but that's truly not mandatory.

I also like that idea. It feels like it should be straight forward to implement that as part of the validation step after the PRs for the schema registry are done. I think it should be possible to add a custom validator in pydantic which can resolve an environment variable.