Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive configuration file reading capabilities to the potato_util library, supporting YAML, JSON, TOML, and INI formats in both synchronous and asynchronous modes. The implementation includes format-specific reader functions plus unified config file readers that automatically detect the file format based on extension.
- Added 5 new sync functions (
read_yaml_file,read_json_file,read_toml_file,read_ini_file,read_config_file) to handle various config formats - Added 5 new async functions (
async_read_yaml_file,async_read_json_file,async_read_toml_file,async_read_ini_file,async_read_config_file) mirroring the sync functionality - Added required dependencies (PyYAML, toml) and updated pre-commit configuration
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/potato_util/io/_sync.py | Implements synchronous config file readers for YAML, JSON, TOML, and INI formats with version-specific TOML handling |
| src/potato_util/io/_async.py | Implements asynchronous config file readers mirroring sync functionality using aiofiles |
| requirements.txt | Adds PyYAML and toml (for Python < 3.11) dependencies |
| .pre-commit-config.yaml | Configures flake8 to ignore E402 errors (import order) for the modified IO files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def async_read_config_file(config_path: str | Path) -> dict[str, Any]: | ||
|
|
There was a problem hiding this comment.
Missing docstring: The async_read_config_file function lacks a docstring explaining its purpose, parameters, return value, and exceptions. All other functions in both files have comprehensive docstrings following the same format.
| async def async_read_config_file(config_path: str | Path) -> dict[str, Any]: | |
| async def async_read_config_file(config_path: str | Path) -> dict[str, Any]: | |
| """Read a configuration file in YAML, JSON, TOML, or INI format. | |
| Args: | |
| config_path (str | Path, required): Path to the configuration file. | |
| Raises: | |
| FileNotFoundError: If the configuration file is not found. | |
| ValueError : If the file format is unsupported. | |
| Exception : If reading the configuration file fails. | |
| Returns: | |
| dict[str, Any]: Configuration file data as a dictionary. | |
| """ |
src/potato_util/io/_sync.py
Outdated
| dict[str, Any]: YAML file data as dictionary. | ||
| """ | ||
|
|
||
| _data: dict[str, str] = {} |
There was a problem hiding this comment.
Type annotation mismatch: _data is declared as dict[str, str] but should be dict[str, Any] to match the return type and the actual data being assigned from yaml.safe_load(), which can contain nested structures with various value types.
| _data: dict[str, str] = {} | |
| _data: dict[str, Any] = {} |
src/potato_util/io/_async.py
Outdated
| async with aiofiles.open(file_path, "r", encoding="utf-8") as _file: | ||
| _content = await _file.read() # type: ignore | ||
|
|
||
| _data = tomllib.loads(_content) or {} |
There was a problem hiding this comment.
Incorrect API usage: The tomllib module (Python 3.11+) only provides a load() method that expects a binary file-like object, not a loads() method. The third-party toml library does have a loads() method. This code will fail at runtime on Python 3.11+ with AttributeError. The implementation should use tomllib.load() with a BytesIO wrapper or decode bytes to string for the toml library fallback.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
src/potato_util/io/_async.py
Outdated
| @validate_call | ||
| async def async_read_config_file(config_path: str | Path) -> dict[str, Any]: | ||
|
|
||
| _config: dict[str, str] = {} |
There was a problem hiding this comment.
Type annotation mismatch: _config is declared as dict[str, str] but should be dict[str, Any] to match the return type and the actual data being returned by the various read functions, which return dict[str, Any].
| _config: dict[str, str] = {} | |
| _config: dict[str, Any] = {} |
|
@bybatkhuu I've opened a new pull request, #10, to work on those changes. Once the pull request is ready, I'll request review from you. |
This pull request adds robust support for reading configuration files in multiple formats (YAML, JSON, TOML, INI) in both synchronous and asynchronous IO utility modules. It introduces new functions for reading each format and a unified function to read any supported config file type, streamlining config file parsing across the codebase. Additionally, it updates the pre-commit configuration to ignore specific linting errors for these files.
Config file reading enhancements:
async_read_yaml_file,async_read_json_file,async_read_toml_file,async_read_ini_file,async_read_config_file) tosrc/potato_util/io/_async.pyfor reading YAML, JSON, TOML, and INI files, plus a unified config file reader supporting all formats. [1] [2]read_yaml_file,read_json_file,read_toml_file,read_ini_file,read_config_file) tosrc/potato_util/io/_sync.pyfor reading YAML, JSON, TOML, and INI files, plus a unified config file reader supporting all formats. [1] [2]Pre-commit configuration update:
.pre-commit-config.yamlto ignore linting error E402 (module imports not at top of file) for the newly modified IO utility files.