Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces nested configuration support by allowing nested Python classes to map to nested sections/objects across supported config formats (INI via dot-separated section names; JSON/YAML/TOML via nested dict navigation). It also updates tests, docs, and examples to use the new parser behavior (including auto-detection based on file extension).
Changes:
- Add nested section name generation in
Configusing class__qualname__(dot notation, stripping<locals>). - Introduce an
IniParseradapter and extendMsgspecParserto navigate dot-separated nested paths. - Add comprehensive nested-config tests and refresh docs/examples to reduce explicit parser setup.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/confkit/config.py |
Builds dot-notated section names for nested classes; switches INI detection to IniParser; introduces _set_parser. |
src/confkit/ext/parsers.py |
Adds IniParser; adds nested navigation + basic type re-parsing for MsgspecParser. |
tests/test_nested_config.py |
New tests covering nested behavior across INI/YAML/JSON/TOML and parser nested navigation. |
tests/test_two_instances.py |
Updates INI parser usage to IniParser and switches to _set_parser. |
tests/test_pydantic_models.py |
Updates fixture/type hints to use IniParser and _set_parser. |
tests/test_on_file_change.py |
Renames test config class and updates references. |
tests/test_multiple_configurations.py |
Uses IniParser and _set_parser for separate config classes. |
tests/test_msgspecparser_no_msgspec.py |
Adds placeholder text related to matching the ImportError message. |
tests/test_metaclass.py |
Uses IniParser and _set_parser. |
tests/test_config_detect_parser.py |
Updates parser detection assertion to expect IniParser. |
tests/test_config_decorators.py |
Uses IniParser in test wrapper setup. |
tests/test_config_classvars.py |
Swaps ConfigParser usage to IniParser and _set_parser. |
tests/test_config.py |
Swaps ConfigParser usage to IniParser and _set_parser. |
docs/usage.md |
Updates quickstart examples to rely on parser auto-detection. |
docs/examples/custom_data_type.md |
Updates snippet text but currently leaves an invalid parser setup line. |
.github/copilot-instructions.md |
Updates internal repo guidance around parser usage (currently references non-existent API/classes). |
examples/basic.py |
Removes explicit parser setup in favor of auto-detection. |
examples/argparse_example.py |
Removes explicit parser setup in favor of auto-detection. |
examples/custom_data_type.py |
Removes explicit parser setup in favor of auto-detection. |
examples/data_types.py |
Removes explicit parser setup in favor of auto-detection. |
examples/decorators.py |
Removes explicit parser setup in favor of auto-detection. |
examples/enums.py |
Removes explicit parser setup in favor of auto-detection. |
examples/file_change_event.py |
Removes explicit parser setup in favor of auto-detection. |
examples/list_types.py |
Removes explicit parser setup in favor of auto-detection. |
examples/multiple_configs.py |
Removes explicit parser setup in favor of auto-detection. |
examples/optional_values.py |
Removes explicit parser setup in favor of auto-detection. |
examples/other_file_types.py |
Updates narrative to rely on auto-detected parser selection. |
examples/pydantic_example.py |
Removes explicit parser setup in favor of auto-detection. |
examples/references.py |
Removes explicit parser setup in favor of auto-detection. |
examples/url_example.py |
Removes explicit parser setup in favor of auto-detection. |
examples/nested_config.py |
New end-to-end example demonstrating nested configuration patterns across formats. |
examples/nested_example.ini |
New sample nested INI file using dot-section notation. |
examples/nested_example.json |
New sample nested JSON file. |
examples/nested_example.toml |
New sample nested TOML file. |
examples/nested_example.yaml |
New sample nested YAML file. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Initial plan * Fix documentation: remove non-existent Parser/set_parser references Co-authored-by: HEROgold <21345384+HEROgold@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: HEROgold <21345384+HEROgold@users.noreply.github.com>
src/confkit/ext/parsers.py
Outdated
| # This will raise ConfigPathConflictError if an intermediate path is a scalar | ||
| section_data = self._navigate_to_section(section, create=True) | ||
| if section_data is not None: | ||
| # Try to preserve the original type by parsing the string value | ||
| # This is important for JSON/YAML/TOML which support native types | ||
| parsed_value = self._parse_value(value) | ||
| section_data[option] = parsed_value |
There was a problem hiding this comment.
MsgspecParser.set() silently no-ops when _navigate_to_section(section, create=True) returns None (this happens when the section path resolves to a scalar rather than a dict at the final element). This can drop writes without any error. Consider raising ConfigPathConflictError when section_data is None so callers learn about the conflict instead of losing updates.
| # This will raise ConfigPathConflictError if an intermediate path is a scalar | |
| section_data = self._navigate_to_section(section, create=True) | |
| if section_data is not None: | |
| # Try to preserve the original type by parsing the string value | |
| # This is important for JSON/YAML/TOML which support native types | |
| parsed_value = self._parse_value(value) | |
| section_data[option] = parsed_value | |
| # This will raise ConfigPathConflictError if an intermediate path is a scalar. | |
| # If the final path element is a scalar, _navigate_to_section returns None and | |
| # we also raise ConfigPathConflictError to avoid silently dropping writes. | |
| section_data = self._navigate_to_section(section, create=True) | |
| if section_data is None: | |
| raise ConfigPathConflictError( | |
| f"Cannot set option '{option}' in section path '{section}': " | |
| "path resolves to a scalar value, not a mapping." | |
| ) | |
| # Try to preserve the original type by parsing the string value | |
| # This is important for JSON/YAML/TOML which support native types | |
| parsed_value = self._parse_value(value) | |
| section_data[option] = parsed_value |
There was a problem hiding this comment.
It's important we never lose data. However it's also okay for us to have silent no-op's on non-existent sections/fields. However it's important that when a section and scalar are defined they are always present and navigatable. This might have filetype specific behavoir however. So keep that in mind
src/confkit/ext/parsers.py
Outdated
| # Try to preserve the original type by parsing the string value | ||
| # This is important for JSON/YAML/TOML which support native types | ||
| parsed_value = self._parse_value(value) | ||
| section_data[option] = parsed_value | ||
|
|
||
| def _parse_value(self, value: str) -> bool | int | float | str: | ||
| """Parse a string value to its appropriate type for structured formats. | ||
|
|
||
| Attempts to convert string values back to their original types: | ||
| - "True"/"False" -> bool | ||
| - Integer strings -> int | ||
| - Float strings -> float | ||
| - Everything else remains a string | ||
| """ | ||
| if value == "True": | ||
| return True | ||
| if value == "False": | ||
| return False | ||
|
|
||
| try: | ||
| return int(value) | ||
| except ValueError: | ||
| pass | ||
|
|
||
| try: | ||
| return float(value) | ||
| except ValueError: | ||
| pass | ||
|
|
There was a problem hiding this comment.
MsgspecParser._parse_value() tries to infer types from the serialized string value (e.g., converting digit-only strings to int). Because Config._set() always passes str(value) to the parser, this will incorrectly coerce string configs like Config("001") or Config("123") into numbers/booleans in JSON/YAML/TOML, losing information (leading zeros, exact casing, etc.). To avoid data corruption, consider changing the parser interface so structured parsers receive the original Python value (or the BaseDataType), rather than guessing types from a string representation.
| # Try to preserve the original type by parsing the string value | |
| # This is important for JSON/YAML/TOML which support native types | |
| parsed_value = self._parse_value(value) | |
| section_data[option] = parsed_value | |
| def _parse_value(self, value: str) -> bool | int | float | str: | |
| """Parse a string value to its appropriate type for structured formats. | |
| Attempts to convert string values back to their original types: | |
| - "True"/"False" -> bool | |
| - Integer strings -> int | |
| - Float strings -> float | |
| - Everything else remains a string | |
| """ | |
| if value == "True": | |
| return True | |
| if value == "False": | |
| return False | |
| try: | |
| return int(value) | |
| except ValueError: | |
| pass | |
| try: | |
| return float(value) | |
| except ValueError: | |
| pass | |
| # Store the value as-is without attempting type inference. | |
| # This avoids corrupting string values like "001" or "123" by | |
| # incorrectly coercing them to numbers/booleans. | |
| parsed_value = self._parse_value(value) | |
| section_data[option] = parsed_value | |
| def _parse_value(self, value: str) -> str: | |
| """Return the value as-is without trying to infer a different type. | |
| Since the parser only receives the serialized string representation | |
| (e.g. via ``str(value)``), any attempt to guess its original Python | |
| type (bool/int/float) can lead to data corruption (for example, | |
| turning ``"001"`` into ``1``). To avoid this, we simply persist the | |
| given value unchanged. | |
| """ |
There was a problem hiding this comment.
determine the flow of data. if it's correct and fits the workflow simply pass the DataType if the workflow doesn't fit this attempt a quick fix using either a generic value on the method. last resort to keeping it a str as is.
.github/copilot-instructions.md
Outdated
| - `ConfkitParser` protocol (`ext/parsers.py`): Protocol defining the parser interface | ||
| - `MsgspecParser` (`ext/parsers.py`): Parser for JSON, YAML, and TOML files | ||
| - `EnvParser` (`ext/parsers.py`): Parser for environment variables and .env files | ||
| - `Parser` facade (`ext/parsers.py`): Unified facade for all configuration file formats (INI, JSON, YAML, TOML, .env) |
There was a problem hiding this comment.
This doc lists a Parser facade in ext/parsers.py, but there is no Parser type exported/implemented there (only ConfkitParser, IniParser, MsgspecParser, EnvParser). Please either add the facade described here or update the documentation to reflect the actual API surface.
| - `Parser` facade (`ext/parsers.py`): Unified facade for all configuration file formats (INI, JSON, YAML, TOML, .env) | |
| - `ConfkitParser` protocol (`ext/parsers.py`): Defines the unified parser interface for all configuration file formats (INI, JSON, YAML, TOML, .env) |
| # TD: match error msg to pytest.raises() | ||
| _ = ( | ||
| r"confkit.ext.parsers requires the optional 'msgspec' extra. " | ||
| r"Install it via 'pip install " | ||
| r"confkit[msgspec]' or 'uv add confkit[msgspec]'." | ||
| r"This is required for json, toml and yaml parsing." | ||
| ) |
There was a problem hiding this comment.
The new regex string assigned to _ is currently unused and the test still uses with pytest.raises(ImportError): without a match=.... Either use this regex in the assertion (so the test validates the message) or remove the dead code/TODO to keep the test focused.
There was a problem hiding this comment.
implement the regex match
examples/nested_config.py
Outdated
| IniConfig.set_parser(IniParser()) | ||
| IniConfig.set_file(Path("nested_example.ini")) |
There was a problem hiding this comment.
This example calls IniConfig.set_parser(IniParser()), but Config.set_parser() is now deprecated and INI parsing is already auto-detected from the .ini extension via set_file(...). Consider removing the explicit set_parser here to avoid demonstrating deprecated API usage in docs/examples.
| @@ -20,10 +24,16 @@ | |||
| ) | |||
| raise ImportError(msg) from exc | |||
There was a problem hiding this comment.
confkit.ext.parsers unconditionally imports msgspec at module import time and raises an ImportError if it's missing. Since Config now imports IniParser from this module, INI-only usage (and importing confkit) will fail unless the optional msgspec extra is installed. Please defer msgspec imports/errors until MsgspecParser is actually used (e.g., lazy import inside MsgspecParser.__init__/read/write or split msgspec-based parsers into a separate module).
There was a problem hiding this comment.
move "builtin" types to confkit.parsers keeping confkit.ext.parsers as a optional install which should still throw exception on import
| def set_section(self, section: str) -> None: | ||
| if section not in self.data: | ||
| self.data[section] = {} | ||
| self._navigate_to_section(section, create=True) |
There was a problem hiding this comment.
MsgspecParser.set_section() calls _navigate_to_section(..., create=True) but ignores its return value. If the target section path already exists as a scalar (e.g., data={"Parent": {"Child": "x"}} then set_section("Parent.Child")), _navigate_to_section returns None and set_section silently succeeds without creating a section. This should raise ConfigPathConflictError to avoid silent failures when a scalar blocks the requested section path.
| self._navigate_to_section(section, create=True) | |
| section_data = self._navigate_to_section(section, create=True) | |
| if section_data is None: | |
| msg = ( | |
| f"Cannot create section '{section}' because the path is blocked by a " | |
| "scalar value." | |
| ) | |
| raise ConfigPathConflictError(msg) |
There was a problem hiding this comment.
see earlier comment about scalar values and reaching sections
|
@copilot open a new pull request to apply changes based on the comments in this thread |
…ecParser type handling and conflict detection (#54) * Initial plan * Move built-in parsers to confkit.parsers, fix type handling and conflict detection Co-authored-by: HEROgold <21345384+HEROgold@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: HEROgold <21345384+HEROgold@users.noreply.github.com>
No description provided.