Refactor #372
Merged
Refactor #372
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We'll be using fakes for unit tests and fakes often have unused arguments so we ignore this error in tests.
This test module will test the high level end to end flows through the package to make sure the refactor hasn't changed any behaviour.
dbatten5
force-pushed
the
refactor
branch
2 times, most recently
from
536438f to
05642d3
Compare
There was a problem hiding this comment.
Pull Request Overview
This refactor introduces a modular architecture for configuration handling by splitting concerns into parsers, a filesystem abstraction, a service layer, and validation. It also replaces legacy source-based config discovery with protocol-driven components and updates tests accordingly.
- Introduces protocols, service layer, parsers (toml/ini/pyproject), disk filesystem abstraction, and a validator.
- Refactors UserConfig to use the new service and parser registry, and updates utils.deep_merge with typed ConfigValues.
- Adds comprehensive unit, integration, and acceptance tests; removes legacy config_sources tests.
Reviewed Changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit_tests/test_utils.py | Updates deep_merge tests to use types.ConfigValues and string keys. Removes tests tied to removed utils/helpers. |
| tests/unit_tests/test_service.py | Adds unit tests for ConfigService find/parse/merge/validate flows using fakes. |
| tests/unit_tests/test_config_validator.py | Adds unit tests for Validator returning model_dump. |
| tests/unit_tests/test_config_reader.py | Adds tests for ConfigParser registry (suffix/stem selection and fallback). |
| tests/unit_tests/test_config.py | Removes old UserConfig tests tied to legacy design. |
| tests/unit_tests/config_sources/test_toml_source.py | Removes tests for removed TomlSource. |
| tests/unit_tests/config_sources/test_pyproject_source.py | Removes tests for removed PyprojectSource. |
| tests/unit_tests/config_sources/test_ini_source.py | Removes tests for removed IniSource. |
| tests/unit_tests/config_sources/test_base_source.py | Removes tests for removed BaseSource. |
| tests/integration_tests/test_disk_filesystem.py | Adds integration tests for DiskFilesystem.get_file_path traversal and absolute paths. |
| tests/integration_tests/test_config.py | Adds integration tests for UserConfig discovery, values, path(s), and validation. |
| tests/integration_tests/parsers/test_toml.py | Adds integration tests for TomlParser behavior. |
| tests/integration_tests/parsers/test_pyproject.py | Adds integration tests for PyprojectParser behavior. |
| tests/integration_tests/parsers/test_ini.py | Adds integration tests for IniParser behavior. |
| tests/acceptance_tests/test_config.py | Adds acceptance tests for end-to-end config discovery, merge, and schema validation. |
| src/maison/utils.py | Removes file discovery helpers; narrows to deep_merge with ConfigValues typing. |
| src/maison/types.py | Introduces ConfigValues recursive type alias. |
| src/maison/service.py | Adds ConfigService orchestrating discovery, parsing, merging, and validation. |
| src/maison/protocols.py | Defines Protocols for Parser, Filesystem, ConfigParser, Validator, and IsSchema. |
| src/maison/parsers/toml.py | Adds TomlParser; returns {} on missing file. |
| src/maison/parsers/pyproject.py | Adds PyprojectParser extracting [tool.] subtree. |
| src/maison/parsers/ini.py | Adds IniParser producing dict of sections/options. |
| src/maison/parsers/init.py | Exposes parser classes in package namespace. |
| src/maison/errors.py | Adds UnsupportedConfigError; retains NoSchemaError/BadTomlError. |
| src/maison/disk_filesystem.py | Adds DiskFilesystem with upward search for files. |
| src/maison/config_validator.py | Adds Validator that instantiates schema and returns model_dump. |
| src/maison/config_parser.py | Adds parser registry keyed by (suffix, optional stem) with fallback. |
| src/maison/config.py | Refactors UserConfig to new service architecture and typing; adds bootstrap. |
| src/maison/config_sources/* | Removes legacy source-based classes and package. |
| .ruff.toml | Adjusts per-file ignores to align with new test and init docstring rules. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/maison/types.py
Outdated
| @@ -0,0 +1,6 @@ | |||
| """Holds type definitions that are used across the pacakge.""" | |||
There was a problem hiding this comment.
Typo in the module docstring: 'pacakge' should be 'package'.
Suggested change
| """Holds type definitions that are used across the pacakge.""" | |
| """Holds type definitions that are used across the package.""" |
src/maison/service.py
Outdated
| @@ -0,0 +1,90 @@ | |||
| """Holds the defintion of the main service class.""" | |||
There was a problem hiding this comment.
Typo in the module docstring: 'defintion' should be 'definition'.
Suggested change
| """Holds the defintion of the main service class.""" | |
| """Holds the definition of the main service class.""" |
| Implements the `Filesystem` protocol. | ||
| """ | ||
|
|
||
| @functools.lru_cache |
There was a problem hiding this comment.
Avoid caching results of filesystem lookups with lru_cache on an instance method; it can return stale paths if files are created/removed between calls and ties cache keys to the instance identity. Remove the @lru_cache decorator, or implement explicit invalidation/TTL if caching is required.
Suggested change
| @functools.lru_cache |
src/maison/parsers/toml.py
Outdated
| """See the Parser.parse_config method.""" | ||
| try: | ||
| return dict(toml.load(file_path)) | ||
| except FileNotFoundError: |
There was a problem hiding this comment.
Invalid TOML (toml.TomlDecodeError) will currently propagate and crash parsing, unlike the missing-file path that returns an empty dict. Either catch toml.TomlDecodeError and return {}, or raise a project-specific error (e.g., errors.BadTomlError) to keep behavior consistent and predictable.
Suggested change
| except FileNotFoundError: | |
| except (FileNotFoundError, toml.TomlDecodeError): |
These files don't need a docstring
This class is used to parse a config using specific e.g. `toml`, `ini` parsers registered against the class.
This class will be used to encapsulate all interaction with the disk filesystem.
This class does all the coordination of business logic when attempting to find and retrieve config values.
For more consistent terminology
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This refactor is aimed at improving the architecture of the package with the following intentions: