Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork95
Use SettingsErrors when file can't be used as source#432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
hramezani commentedOct 2, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
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. |
It seems to me that the change would not break users code because
so anybody doing: try:s=Settings()exceptValueError:# 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 ? |
pydantic_settings/sources.py Outdated
| try: | ||
| vars.update(self._read_file(file_path)) | ||
| except ValueError as e: | ||
| raise SettingsError(f'Parsing error encountered for {file_path}: {e}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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), theValueError,which is aboutdict.update(), is not very informative:
>>>d{}>>>d.update("abc")Traceback (mostrecentcalllast):File"<stdin>",line1,in<module>ValueError:dictionaryupdatesequenceelement#0 has length 1; 2 is required
Also, I realize that callingdict.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we can probably check if the object returned byself._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:
importtomlitomli.loads("demo =\ndemo2 =")
Will throw a tomli.TOMLDecodeError :/ finally i think we should simply keep theValueError.
hramezani commentedOct 4, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks@dlax for the explanation. However, it is still a breaking change because some people's code might depend on the error type
|
@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.
|
e49dbbd to4a9f0c3Compare| try: | ||
| settings = self._read_file(file_path) | ||
| except ValueError as e: | ||
| raise SettingsError(f'Failed to parse settings from {file_path}, {e}') | ||
| if not isinstance(settings, dict): | ||
| raise SettingsError( | ||
| f'Failed to parse settings from {file_path}, expecting an object (valid dictionnary)' | ||
| ) | ||
| vars.update(settings) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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 raisingSettingsError; then, we can merge the tests only.
2- Keep the PR open untilV3 and merge it beforeV3.
I would prefer option 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@l00ptr no need for a new PR.
Then we have to wait to merge it onV3. I will add av3 label to the PR to not merge it untilv3
Some file formats (at least yaml) allow to represent non dictionnaryvalues. In such situation we can't add the values read from those files.Instead of raising a ValueError, we now raise a SettingsError andindicate we can't parse the related config file.
4a9f0c3 to55ff1e5CompareWe verify the loaded settings from files is dict, otherwise we raise aSettingsError. With that change, we make Pydantic fails a bit faster.
55ff1e5 toc6e6fb9Compare
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.