Issue 192 #320
Closed
Issue 192 #320
+615
−14
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
Reviewer's GuideAdds TOML key-handling improvements (allowing quoted period-containing keys), wires TOML section headers into the multisource configuration loader, introduces a reusable builtins.open patching utility for tests, extends TOML-related test coverage, updates the clean script with a no-upgrades flag, bumps the library version to 0.9.1, and documents changes in the changelog. Sequence diagram for TOML configuration loading with section headers and period-containing keyssequenceDiagram
actor Client
participant Loader as MultisourceConfigurationLoader
participant Options as MultisourceConfigurationLoaderOptions
participant Props as ApplicationProperties
participant TomlLoader as ApplicationPropertiesTomlLoader
Client->>Loader: _load_config(file_name, options, Props, handle_error_fn)
Loader->>Loader: determine config_file_type == TOML
Loader->>Loader: __load_as_toml(file_name, options, Props, handle_error_fn)
Loader->>TomlLoader: load_and_set(Props, file_name, section_header=options.section_header_if_toml, handle_error_fn, clear_property_map=False, check_for_file_presence=True, strict_mode=True)
TomlLoader->>TomlLoader: parse TOML file
TomlLoader->>TomlLoader: extract configuration_map
alt configuration_map is not empty
TomlLoader->>Props: load_from_dict(configuration_map, clear_map=clear_property_map, allow_periods_in_keys=True)
Props->>Props: clear() if clear_map
Props->>Props: __scan_map(config_map, current_prefix="", allow_periods_in_keys=True)
end
TomlLoader-->>Loader: (did_apply_map, did_have_one_error)
Loader-->>Client: (did_apply_map, did_have_one_error)
Class diagram for updated configuration loading and TOML key handlingclassDiagram
class ApplicationProperties {
- Dict[Any, Any] __flat_property_map
- str __assignment_operator
- str __separator
+ clear() void
+ load_from_dict(config_map: Dict[Any, Any], clear_map: bool, allow_periods_in_keys: bool) void
+ verify_full_part_form(property_key: str) str
+ property_names_under(key_name: str) List[str]
- __scan_map(config_map: Dict[Any, Any], current_prefix: str, allow_periods_in_keys: bool) void
}
class ApplicationPropertiesTomlLoader {
+ load_and_set(properties_object: ApplicationProperties, file_name: str, section_header: Optional[str], handle_error_fn: Callable[[str, Optional[Exception]], None], clear_property_map: bool, check_for_file_presence: bool, strict_mode: bool) Tuple[bool, bool]
}
class MultisourceConfigurationLoaderOptions {
+ bool allow_json5
+ Optional[str] section_header_if_toml
}
class MultisourceConfigurationLoader {
- __load_as_toml(file_name: str, options: MultisourceConfigurationLoaderOptions, application_properties: ApplicationProperties, handle_error_fn: Callable[[str, Optional[Exception]], None]) Tuple[bool, bool]
- _load_config(file_name: str, options: MultisourceConfigurationLoaderOptions, application_properties: ApplicationProperties, handle_error_fn: Callable[[str, Optional[Exception]], None]) Tuple[bool, bool]
}
ApplicationPropertiesTomlLoader --> ApplicationProperties : uses
MultisourceConfigurationLoader --> ApplicationPropertiesTomlLoader : calls
MultisourceConfigurationLoader --> MultisourceConfigurationLoaderOptions : reads
MultisourceConfigurationLoaderOptions --> ApplicationPropertiesTomlLoader : passes section_header
ApplicationPropertiesTomlLoader --> ApplicationProperties : load_from_dict(allow_periods_in_keys=True)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
ApplicationProperties.__scan_map, the logic that always wraps keys containing the separator in quotes might be surprising whenallow_periods_in_keysis False (the earlier validation already rejects such keys); consider only quoting keys whenallow_periods_in_keysis True or refactoring to keep the behavior clearer. - The validation error message in
__scan_mapstill states that keys cannot contain a '.' character, even though TOML loading now allows quoted periods viaallow_periods_in_keys; it would be clearer to align the message with the new behavior or make it context-specific. - In
PatchBuiltinOpen.register_exception_for_file,exception_maponly stores a single(mode, exception)tuple per file, so registering multiple exceptions for the same path will silently overwrite previous entries; if multiple mode/exception pairs per file are needed, consider using a nested map keyed by mode.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ApplicationProperties.__scan_map`, the logic that always wraps keys containing the separator in quotes might be surprising when `allow_periods_in_keys` is False (the earlier validation already rejects such keys); consider only quoting keys when `allow_periods_in_keys` is True or refactoring to keep the behavior clearer.
- The validation error message in `__scan_map` still states that keys cannot contain a '.' character, even though TOML loading now allows quoted periods via `allow_periods_in_keys`; it would be clearer to align the message with the new behavior or make it context-specific.
- In `PatchBuiltinOpen.register_exception_for_file`, `exception_map` only stores a single `(mode, exception)` tuple per file, so registering multiple exceptions for the same path will silently overwrite previous entries; if multiple mode/exception pairs per file are needed, consider using a nested map keyed by mode.
## Individual Comments
### Comment 1
<location> `test/patches/patch_builtin_open.py:58-1` </location>
<code_context>
+ f"register_text_content[{exact_file_name}]=[{file_contents}]]"
+ )
+
+ def register_binary_content_for_file(
+ self, exact_file_name: str, file_contents: bytes
+ ) -> None:
+ """
+ Register text content to return when the specified file is opened for reading as
+ a text file.
</code_context>
<issue_to_address>
**nitpick:** Docstring for `register_binary_content_for_file` is misleading (mentions text instead of binary).
Since this helper accepts `bytes` and is used with "rb" mode, the docstring should describe registering binary content for binary reads, not text. Please update the wording accordingly to avoid confusion and potential misuse.
</issue_to_address>
### Comment 2
<location> `newdocs/src/changelog.md:90-91` </location>
<code_context>
+ - added ability to translate from string to other forms
+ - controlled by flag, and only occurs on pure strings, not any strings with
+ any typing
+ - i.e. only manual set and "config" files are untyped, so only those are
+ affected
+- [Issue 145](https://github.com/jackdewinter/application_properties/issues/145)
</code_context>
<issue_to_address>
**issue (typo):** Replace `manual set` with `manually set` for correct grammar.
Update the phrase to: `only manually set and "config" files are untyped` for correct grammar and clarity.
```suggestion
- i.e. only manually set and "config" files are untyped, so only those are
affected
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -0,0 +1,157 @@ | |||
| """ | |||
There was a problem hiding this comment.
nitpick: Docstring for register_binary_content_for_file is misleading (mentions text instead of binary).
Since this helper accepts bytes and is used with "rb" mode, the docstring should describe registering binary content for binary reads, not text. Please update the wording accordingly to avoid confusion and potential misuse.
Comment on lines
+90
to
+91
| - i.e. only manual set and "config" files are untyped, so only those are | ||
| affected |
There was a problem hiding this comment.
issue (typo): Replace manual set with manually set for correct grammar.
Update the phrase to: only manually set and "config" files are untyped for correct grammar and clarity.
Suggested change
| - i.e. only manual set and "config" files are untyped, so only those are | |
| affected | |
| - i.e. only manually set and "config" files are untyped, so only those are | |
| affected |
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.
Summary by Sourcery
Allow TOML configurations to use quoted keys containing periods and ensure TOML section headers are honored when loading configuration files, while updating tooling, tests, and documentation for the new behavior.
New Features:
section_header_if_tomlsupport inMultisourceConfigurationLoaderOptionsto control which TOML section is loaded from configuration files.builtins.opento simplify testing file I/O behavior.Bug Fixes:
pyproject.tomlfiles.clean.shto skip dependency upgrade checks via a new--no-upgradesflag.Enhancements:
ApplicationProperties.load_from_dictto optionally allow periods in keys and preserve them as quoted segments in flattened property names.Documentation:
Tests:
pyproject.tomland explicit config files.openin tests to validate TOML loading without touching the real filesystem.