Conversation
…usly and synchronously
There was a problem hiding this comment.
Pull request overview
Adds new APIs to load and merge configuration from multiple files/directories in both sync and async workflows, with a shared enum to define supported config formats.
Changes:
- Introduces
ConfigFileFormatEnum(YAML/JSON/TOML/INI) undersrc/potato_util/constants/_enum.py. - Adds
read_all_configs(sync) andasync_read_all_configs(async) to read and deep-merge all matching config files from one or more directories. - Exports the new functions via each module’s
__all__.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/potato_util/io/_sync.py | Adds read_all_configs that globs config files in directories and deep-merges them. |
| src/potato_util/io/_async.py | Adds async_read_all_configs async counterpart, including new imports and merge behavior. |
| src/potato_util/constants/_enum.py | Adds ConfigFileFormatEnum and exports it via __all__. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not os.path.isabs(_config_dir): | ||
| _current_dir = await aiofiles.os.getcwd() | ||
| _config_dir = os.path.join(_current_dir, _config_dir) | ||
|
|
||
| if await aiofiles.os.path.isdir(_config_dir): | ||
| if ConfigFileFormatEnum.YAML in allowed_formats: |
There was a problem hiding this comment.
Non-existent / non-directory entries in configs_dir are silently ignored (if await aiofiles.os.path.isdir(_config_dir): ...). This makes typos in config paths easy to miss and can lead to an empty config dict without any signal. Consider raising FileNotFoundError (or at least logging a warning) when a provided directory is missing.
| _file_paths.sort() | ||
| for _file_path in _file_paths: | ||
| _config_data = read_config_file(config_path=_file_path) | ||
| _config_dict = deep_merge(_config_dict, _config_data) | ||
|
|
There was a problem hiding this comment.
_file_paths.sort() globally reorders files across all provided directories. Because merge order affects precedence (later files overwrite earlier ones via deep_merge), this may not match the priority implied by the configs_dir list. Consider merging per-directory in the order provided (sorting within each dir if needed) and document the precedence rules.
| if not os.path.isabs(_config_dir): | ||
| _current_dir = os.getcwd() | ||
| _config_dir = os.path.join(_current_dir, _config_dir) | ||
|
|
||
| if os.path.isdir(_config_dir): | ||
| if ConfigFileFormatEnum.YAML in allowed_formats: |
There was a problem hiding this comment.
Non-existent / non-directory entries in configs_dir are silently ignored (if os.path.isdir(_config_dir): ...). This can hide misconfiguration (e.g., path typos) and return an empty dict without any signal. Consider raising FileNotFoundError (or logging a warning) when a provided directory is missing.
| if await aiofiles.os.path.isdir(_config_dir): | ||
| if ConfigFileFormatEnum.YAML in allowed_formats: | ||
| _file_paths.extend(glob.glob(os.path.join(_config_dir, "*.yaml"))) | ||
| _file_paths.extend(glob.glob(os.path.join(_config_dir, "*.yml"))) | ||
|
|
||
| if ConfigFileFormatEnum.JSON in allowed_formats: | ||
| _file_paths.extend(glob.glob(os.path.join(_config_dir, "*.json"))) | ||
|
|
||
| if ConfigFileFormatEnum.TOML in allowed_formats: | ||
| _file_paths.extend(glob.glob(os.path.join(_config_dir, "*.toml"))) | ||
|
|
||
| if ConfigFileFormatEnum.INI in allowed_formats: | ||
| _file_paths.extend(glob.glob(os.path.join(_config_dir, "*.ini"))) | ||
| _file_paths.extend(glob.glob(os.path.join(_config_dir, "*.cfg"))) | ||
|
|
There was a problem hiding this comment.
In this async function, glob.glob(...) is a synchronous directory scan and will block the event loop for large directories. Consider running the globbing in a thread (e.g., asyncio.to_thread) or using an async-friendly directory walk so async_read_all_configs stays non-blocking.
| _file_paths.sort() | ||
| for _file_path in _file_paths: | ||
| _config_data = await async_read_config_file(config_path=_file_path) | ||
| _config_dict = deep_merge(_config_dict, _config_data) | ||
|
|
There was a problem hiding this comment.
_file_paths.sort() globally reorders files across all provided directories. Since merge order determines precedence (later files overwrite earlier ones via deep_merge), this can unintentionally change the priority implied by the configs_dir list. Consider preserving the directory order (sort within each directory, or merge per-directory in the order passed) and document the precedence rules.
This pull request adds new functionality for reading and merging multiple configuration files in both asynchronous and synchronous workflows, and introduces a new enum to standardize supported config formats. The changes improve flexibility when loading configuration data from directories, supporting multiple formats and merging them into a single dictionary.
Config format support:
ConfigFileFormatEnumtosrc/potato_util/constants/_enum.py, defining supported config file formats: YAML, JSON, TOML, and INI.Asynchronous config reading enhancements (
src/potato_util/io/_async.py):async_read_all_configs, an async function to read and merge all config files from one or more directories, supporting multiple formats as specified byConfigFileFormatEnum.globfor file matching anddeep_mergefor merging configs. [1] [2]async_read_all_configsin the module’s__all__list for public API access.Synchronous config reading enhancements (
src/potato_util/io/_sync.py):read_all_configs, a synchronous function mirroring the async version, to read and merge config files from directories, supporting the same formats.globanddeep_merge. [1] [2]read_all_configsin the module’s__all__list for public API access.