Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This pull request introduces configuration management for the render-engine API, handling site settings from pyproject.toml. The PR includes several significant changes:
Changes:
- Adds new
ApiConfigclass with lazy-loaded configuration frompyproject.toml - Migrates from Python 3.12+ to Python 3.10+ support with conditional tomli/tomllib imports
- Adds comprehensive testing infrastructure including nox for multi-version testing and hypothesis for property-based tests
- Replaces
tomldependency withrichfor console output andtomlifor Python <3.11
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| render_engine_api/config.py | New config class with lazy loading and TOML parsing from pyproject.toml |
| tests/test_config.py | Comprehensive test suite using pytest and hypothesis for config validation |
| pyproject.toml | Updated Python version requirement to >=3.10, added dependencies (rich, tomli), and test/lint configurations |
| uv.lock | Lock file updates for new dependencies including nox, hypothesis, rich, and associated packages |
| noxfile.py | New nox configuration for testing across Python 3.10-3.14 |
| render-engine-api/config.py | Removed old CliConfig implementation |
| render-engine-api/site.py | Removed site utility functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _module: str | None = None | ||
| _site: str | None = None | ||
| _collection: str | None = None | ||
| _config_loaded: bool = False | ||
| config_file: str | Path | None = CONFIG_FILE_NAME |
There was a problem hiding this comment.
The dataclass uses mutable default values for private attributes. While None, False, and instances are safe, if this pattern is extended to mutable types like lists or dicts in the future, it could cause bugs. Consider using field(default_factory=...) from dataclasses for any future mutable defaults.
| _config_loaded: bool = False | ||
| config_file: str | Path | None = CONFIG_FILE_NAME | ||
|
|
||
| def __post_init__(self): |
There was a problem hiding this comment.
The config_file parameter accepts str | Path | None, but when passed as a string, it won't be converted to a Path object before use in open(). While open() accepts both strings and Path objects, it's better to normalize config_file to a Path in __post_init__ for consistency and to enable Path-specific methods. Add: if isinstance(self.config_file, str): self.config_file = Path(self.config_file) in __post_init__.
| def __post_init__(self): | |
| def __post_init__(self): | |
| if isinstance(self.config_file, str): | |
| self.config_file = Path(self.config_file) |
There was a problem hiding this comment.
@brass75 what do you think about this suggestion?
We aren't doing anything other than reading the path so I'm not too concerned.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| "nox", | ||
| "pytest", | ||
| "pytest-cov", | ||
| "hypothesis", |
There was a problem hiding this comment.
Should at least pin >= 6.0.0
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Adds config for API. This manages the settings configuration of a site.
Also included are the setting for nox and pytest. This was in order to accurately test
config.pyfor all version in light of not having GitHub Actions not setup yet.There are also some changes to pyproject.toml for fixes for Type hinting (ty) and deptry.
Co-Authored with Claude as a troubleshooting tool and for initial building of tests.