Feature/dev 75 add changelog path to toml config - #73
Conversation
…ngelog_path - to make this work we will need a rework of the default handling in config base
…aths in FileConfig
There was a problem hiding this comment.
Pull request overview
This pull request adds support for configuring changelog paths in the TOML configuration file. It introduces a new changelog_paths field to the FileConfig class that accepts either a string or list of strings, along with an optional default changelog path setting.
Changes:
- Added
changelog_pathsand_default_changelog_pathattributes toFileConfigwith helper parsing functions - Refactored
_get_typefunction to remove thedefaultparameter and updated related getter functions - Added new
get_str_or_str_listfunction to handle string or list of strings configuration values - Updated test assertions to match new error message format
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/devops/config/config_file.py | Added changelog path configuration fields and parsing functions to FileConfig |
| src/devops/config/base.py | Refactored getter functions to consistently handle default values and added get_str_or_str_list utility |
| tests/config/test_config.py | Updated test assertions to match new error messages from refactored _get_type function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| The parsed list of changelog paths. | ||
| """ | ||
| changelog_paths = get_str_or_str_list( | ||
| table, "changelog_path", FileConfig.changelog_path |
There was a problem hiding this comment.
The code references FileConfig.changelog_path (singular), but the actual attribute defined in the FileConfig class is changelog_paths (plural). This will cause an AttributeError at runtime. Change FileConfig.changelog_path to FileConfig.changelog_paths.
| table, "changelog_path", FileConfig.changelog_path | |
| table, "changelog_path", FileConfig.changelog_paths |
| lines.append(f'#encoding = "{self.encoding}"\n') | ||
| paths_str = '", "'.join(str(p) for p in self.changelog_paths) | ||
| lines.append(f'#changelog_paths = ["{paths_str}"]\n') | ||
| lines.append(f'#_default_changelog_path = "{self.default_changelog_path}"\n') |
There was a problem hiding this comment.
The _default_changelog_path field is an internal implementation detail (indicated by the leading underscore) and should not be exposed in the TOML configuration output. Users should only configure changelog_paths and optionally default_changelog_path (without the underscore). Consider removing this line or changing it to default_changelog_path if this is intended to be a user-facing configuration option.
| lines.append(f'#_default_changelog_path = "{self.default_changelog_path}"\n') | |
| lines.append(f'#default_changelog_path = "{self.default_changelog_path}"\n') |
| return get_str(mapping, key, default) | ||
|
|
||
| if isinstance(value, list) and all(isinstance(item, str) for item in value): | ||
| return get_str_list(mapping, key, default) |
There was a problem hiding this comment.
The function performs redundant lookups by calling get_str or get_str_list after already retrieving the value. Instead of re-querying the mapping, directly return the validated value to avoid duplicate lookups and potential inconsistencies. The current implementation also doesn't handle the case where the default is provided and used as the value - it will be passed to nested functions unnecessarily.
| return get_str(mapping, key, default) | |
| if isinstance(value, list) and all(isinstance(item, str) for item in value): | |
| return get_str_list(mapping, key, default) | |
| return value | |
| if isinstance(value, list) and all(isinstance(item, str) for item in value): | |
| return value |
| table, "changelog_path", FileConfig.changelog_path | ||
| ) | ||
|
|
||
| if isinstance(changelog_paths, str): | ||
| changelog_paths = [changelog_paths] | ||
|
|
There was a problem hiding this comment.
This normalization logic is redundant because get_str_or_str_list already returns either a string or list. If you want the result to always be a list, this conversion should happen, but the function name and logic suggest it can return either type. Consider returning the value directly and removing this check, or ensure the contract is clear about always returning a list from this function.
| table, "changelog_path", FileConfig.changelog_path | |
| ) | |
| if isinstance(changelog_paths, str): | |
| changelog_paths = [changelog_paths] | |
| table, | |
| "changelog_path", | |
| FileConfig.changelog_paths, | |
| ) |
| """ | ||
| encoding = get_str(table, "encoding", default=FileConfig.encoding) | ||
|
|
||
| ### Validate encoding |
There was a problem hiding this comment.
Use double hash ## for section comments rather than triple hash ### to maintain consistency with Python documentation conventions. Triple hash is typically reserved for major section headers in documentation.
| ### Validate encoding | |
| ## Validate encoding |
No description provided.