Skip to content

Conversation

@mahenzon
Copy link

@mahenzon mahenzon commented Dec 2, 2025

Open files (JSON, TOML, YAML) using file_path.open() instead of open(file_path).

Not only this is more suitable way, this change also adds support for passing types other than pathlib.Path and str, for example Traversable and some others, that don't have real path to file, but allow to be opened using the .open() method.

@Sube-py
Copy link

Sube-py commented Dec 2, 2025

Since you're already using path, why not remove the with statement and use read_bytes instead?

@mahenzon
Copy link
Author

mahenzon commented Dec 2, 2025

Since you're already using path, why not remove the with statement and use read_bytes instead?

json.load, yaml.safe_load, tomllib.load expect generic IO that can be read as file. We don't want to pass raw bytes, let the library do the reading when it needs to.

@mahenzon mahenzon changed the title Open files using Path.open method Add non-Path files support (for example Traversable) and open files using Path.open method Dec 2, 2025
@mahenzon
Copy link
Author

mahenzon commented Dec 2, 2025

I've added tests for Traversable type to show the usecase.

@hramezani hramezani requested a review from Copilot December 2, 2025 13:15
Copilot finished reviewing on behalf of hramezani December 2, 2025 13:19
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves file handling in pydantic-settings by switching from the built-in open() function to using the .open() method on path-like objects. This change aims to add support for non-filesystem path types like Traversable from importlib.resources, enabling configuration files to be loaded from packages and archives.

Key Changes:

  • Modified file opening in JSON, YAML, and TOML config sources to use path.open() instead of open(path)
  • Updated _read_files() logic in ConfigFileSourceMixin to handle path-like objects more flexibly
  • Added test for Traversable support using importlib.resources

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pydantic_settings/sources/providers/json.py Changed to use file_path.open() for opening JSON files
pydantic_settings/sources/providers/yaml.py Changed to use file_path.open() for opening YAML files
pydantic_settings/sources/providers/toml.py Changed to use file_path.open() for opening TOML files
pydantic_settings/sources/base.py Modified _read_files() to handle non-Path objects by conditionally converting strings and applying expanduser() only to Path objects
tests/test_source_json.py Added test_traversable_support() to verify Traversable objects can be used as configuration file sources

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +203 to +206
if isinstance(file, str):
file_path = Path(file)
else:
file_path = file
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic has a bug when handling Traversable objects. The check at line 199 isinstance(files, (str, os.PathLike)) doesn't catch Traversable objects (they don't implement os.PathLike). This means a single Traversable object won't be converted to a list, and the iteration at line 202 will fail or behave incorrectly.

Consider changing the logic to check if files is not a sequence (or more specifically, check if it has the file-like interface you need) before wrapping it in a list. For example:

if not isinstance(files, Sequence) or isinstance(files, (str, os.PathLike)):
    files = [files]

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sube-py would be great if you could handle this

@pydantic pydantic deleted a comment from Copilot AI Dec 2, 2025
file_path = Path(file)
else:
file_path = file
if isinstance(file_path, Path):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this if here? As we convert str to Path above?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants