Use SettingsErrors when file can't be used as source

[l00ptr]

Some file formats (at least yaml) allow to represent non dictionnary values. In such situation we can't add the values read from those files. Instead of raising a ValueError, we now raise a SettingsError and indicate we can't parse the related config file.

[hramezani]

Thanks @l00ptr for this PR. I think this will introduce a breaking change because some people's code might depend on the old error.

This change can be made in the next major release V3. but we need to have consistent behavior over all settings sources and add some tests for that.

[l00ptr]

ok, i think i can try to help on that topic. I can start writing a simple test case for invalid yaml file and handle json decoding error.

[dlax]

I think this will introduce a breaking change because some people's code might depend on the old error.

It seems to me that the change would not break users code because SettingsError is a subclass of ValueError:

pydantic-settings/pydantic_settings/sources.py

Line 122 in 0d605d0

class SettingsError(ValueError):

so anybody doing:

try:
   s = Settings()
except ValueError:
   # handle exception here
   ...

would still have their code working. This could be checked in tests added here.

Or did you have something else in mind @hramezani ?

[dlax]

I think we could improve the user experience by explicitly checking that the result of file read is a dict because otherwise, (e.g. in added tests), the ValueError, which is about dict.update(), is not very informative:

>>> d
{}
>>> d.update("abc")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: dictionary update sequence element #0 has length 1; 2 is required

Also, I realize that calling dict.update() with an iterable is also valid and so loading a YAML like:

- - x
  - y
- - a
  - b

would work; not sure if it's expected? But maybe it's another issue.

[l00ptr]

we can probably check if the object returned by self._read_file is a dict or not and handle that specific case.

There are also a lot of cases where reading from (toml, yaml,...) file will raise ValueError. Eg:

import tomli

tomli.loads("demo =\ndemo2 =")

Will throw a tomli.TOMLDecodeError :/ finally i think we should simply keep the ValueError.

[hramezani]

I think this will introduce a breaking change because some people's code might depend on the old error.

It seems to me that the change would not break users code because SettingsError is a subclass of ValueError:

pydantic-settings/pydantic_settings/sources.py

Line 122 in 0d605d0

class SettingsError(ValueError):

so anybody doing:

try:
   s = Settings()
except ValueError:
   # handle exception here
   ...

would still have their code working. This could be checked in tests added here.

Or did you have something else in mind @hramezani ?

Thanks @dlax for the explanation. However, it is still a breaking change because some people's code might depend on the error type ValueError(rare case). So, better not to do it now and postpone the decision before V3.

pydantic-settings had a lot of changes from V1 to V2 and we decided not to introduce breaking changes even something like this.

[hramezani]

@l00ptr Thanks for your update and for adding tests.

As I mentioned above, we can't merge this PR because of breaking changes. but you've added some good tests.
Please update the PR, keep only the tests, and move them to the specific test file.

• test_source_json.py
• test_source_toml.py
• test_source_yaml.py

[hramezani]

@l00ptr Thanks for updating this.
As I mentioned before we can't merge this PR because it will introduce a breaking change. So, we have 2 options here:

1- Keep only the tests that test the current behavior(ValueError) and revert to raising SettingsError; then, we can merge the tests only.
2- Keep the PR open until V3 and merge it before V3.

I would prefer option 1.

[l00ptr]

@hramezani i splitted those changes on 3 commits, let me know if i should create a specific MR for the 2 last commits (about SettingsError) and only keep the first one here.

[hramezani]

@l00ptr no need for a new PR.

Then we have to wait to merge it on V3. I will add a v3 label to the PR to not merge it until v3