Issue 192 #321
Merged
Issue 192 #321
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 (support for quoted keys containing periods and slashes), wires section-header handling into the multisource configuration loader for TOML files, introduces reusable open() patching utilities for tests, extends TOML loader tests, updates CI tooling script flags, bumps version to 0.9.1, and documents fixes in the changelog. Sequence diagram for TOML configuration loading with section headers and quoted keyssequenceDiagram
participant Client
participant Loader as MultisourceConfigurationLoader
participant Options as MultisourceConfigurationLoaderOptions
participant AppProps as ApplicationProperties
participant TomlLoader as ApplicationPropertiesTomlLoader
Client->>Loader: _load_config(file_name, options, AppProps, handle_error_fn)
Loader->>Loader: detect ConfigurationFileType TOML
Loader->>Loader: __load_as_toml(file_name, options, AppProps, handle_error_fn)
Loader->>TomlLoader: load_and_set(AppProps, file_name, section_header=Options.section_header_if_toml, handle_error_fn, clear_property_map=False, check_for_file_presence=True, is_configuration_required=True)
TomlLoader->>AppProps: load_from_dict(configuration_map, clear_map=False, allow_periods_in_keys=True)
AppProps->>AppProps: __scan_map(config_map, current_prefix="", allow_periods_in_keys=True)
AppProps-->>TomlLoader: properties updated with quoted period keys preserved
TomlLoader-->>Loader: did_apply_map, did_have_one_error
Loader-->>Client: did_apply_map, did_have_one_error
Class diagram for updated TOML loading and key parsingclassDiagram
class ApplicationProperties {
+load_from_dict(config_map, clear_map, allow_periods_in_keys)
-__scan_map(config_map, current_prefix, allow_periods_in_keys)
}
class ApplicationPropertiesTomlLoader {
+load_and_set(properties_object, configuration_filename, section_header, handle_error_fn, clear_property_map, check_for_file_presence, is_configuration_required) Tuple~bool,bool~
}
class MultisourceConfigurationLoaderOptions {
+bool parse_json_as_json5
+Optional~string~ section_header_if_toml
}
class MultisourceConfigurationLoader {
-__load_as_toml(file_name, options, application_properties, handle_error_fn) Tuple~bool,bool~
-_load_config(file_name, options, application_properties, handle_error_fn) Tuple~bool,bool~
}
MultisourceConfigurationLoader --> MultisourceConfigurationLoaderOptions : uses
MultisourceConfigurationLoader --> ApplicationPropertiesTomlLoader : calls
ApplicationPropertiesTomlLoader --> ApplicationProperties : config_target
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 4 issues, and left some high level feedback:
- In
PatchBuiltinOpen.__my_open, callingself.stop()and thenself.start()around the passthroughopencall means the side effect is only attached the first time; subsequentstart()calls don’t re-register__my_open, so later opens will be handled by a plain mock instead of the custom logic—consider either avoiding stop/start in the side-effect or reapplying the side effect each timestart()is called. - The key validation error raised in
ApplicationProperties.__scan_mapstill claims keys cannot contain the separator character even whenallow_periods_in_keys=True; consider adjusting the message (or branching it) so it accurately reflects the allowed characters in that mode. - The docstring for
register_binary_content_for_fileinPatchBuiltinOpendescribes registering text content for text mode, but the implementation targets binary content forrbmode—updating this description would make the intent and usage clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PatchBuiltinOpen.__my_open`, calling `self.stop()` and then `self.start()` around the passthrough `open` call means the side effect is only attached the first time; subsequent `start()` calls don’t re-register `__my_open`, so later opens will be handled by a plain mock instead of the custom logic—consider either avoiding stop/start in the side-effect or reapplying the side effect each time `start()` is called.
- The key validation error raised in `ApplicationProperties.__scan_map` still claims keys cannot contain the separator character even when `allow_periods_in_keys=True`; consider adjusting the message (or branching it) so it accurately reflects the allowed characters in that mode.
- The docstring for `register_binary_content_for_file` in `PatchBuiltinOpen` describes registering text content for text mode, but the implementation targets binary content for `rb` mode—updating this description would make the intent and usage clearer.
## Individual Comments
### Comment 1
<location> `application_properties/application_properties.py:472-476` </location>
<code_context>
or "\t" in next_key
or "\n" in next_key
or ApplicationProperties.__assignment_operator in next_key
- or ApplicationProperties.__separator in next_key
+ or (
+ ApplicationProperties.__separator in next_key
+ and not allow_periods_in_keys
</code_context>
<issue_to_address>
**suggestion:** Make the validation error message reflect the `allow_periods_in_keys` behavior.
The `ValueError` text still states that keys cannot contain the separator, even though the condition now allows it when `allow_periods_in_keys` is `True`. To avoid confusion when reading the code, please update the message to reflect that this restriction is conditional (e.g., branch the message on `allow_periods_in_keys` or reword it to note the configurability).
```suggestion
raise ValueError(
"Key strings cannot contain a whitespace character, "
+ f"a '{ApplicationProperties.__assignment_operator}' character, or "
+ f"a '{ApplicationProperties.__separator}' character when periods in keys are disabled."
)
```
</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):** Minor grammar issue: "only manual set" should likely be "only manually set."
Suggest updating the parenthetical to: `i.e. only manually set and "config" files are untyped, so only those are affected`.
```suggestion
- i.e. only manually set and "config" files are untyped, so only those are
affected
```
</issue_to_address>
### Comment 3
<location> `newdocs/src/changelog.md:105` </location>
<code_context>
+<!-- pyml disable-next-line no-duplicate-heading-->
+### Fixed and Added
+
+- Added `py.typed` file to ensure that type hints are consumed by package.
+- Added `__all__` variable to `__init__.py` to resolve mypy error
+
</code_context>
<issue_to_address>
**suggestion (typo):** Minor grammar: add an article before "package."
Consider changing this to "consumed by the package" for more natural grammar.
```suggestion
- Added `py.typed` file to ensure that type hints are consumed by the package.
```
</issue_to_address>
### Comment 4
<location> `newdocs/src/changelog.md:106` </location>
<code_context>
+### Fixed and Added
+
+- Added `py.typed` file to ensure that type hints are consumed by package.
+- Added `__all__` variable to `__init__.py` to resolve mypy error
+
+## Version 0.5.1 - Date: 2022-04-01
</code_context>
<issue_to_address>
**suggestion (typo):** Minor grammar: consider adding an article before "mypy error."
Change this to “to resolve a mypy error” for smoother grammar.
```suggestion
- Added `__all__` variable to `__init__.py` to resolve a mypy error
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
472
to
476
| raise ValueError( | ||
| "Key strings cannot contain a whitespace character, " | ||
| + f"a '{ApplicationProperties.__assignment_operator}' character, or " | ||
| + f"a '{ApplicationProperties.__separator}' character." | ||
| ) |
There was a problem hiding this comment.
suggestion: Make the validation error message reflect the allow_periods_in_keys behavior.
The ValueError text still states that keys cannot contain the separator, even though the condition now allows it when allow_periods_in_keys is True. To avoid confusion when reading the code, please update the message to reflect that this restriction is conditional (e.g., branch the message on allow_periods_in_keys or reword it to note the configurability).
Suggested change
| raise ValueError( | |
| "Key strings cannot contain a whitespace character, " | |
| + f"a '{ApplicationProperties.__assignment_operator}' character, or " | |
| + f"a '{ApplicationProperties.__separator}' character." | |
| ) | |
| raise ValueError( | |
| "Key strings cannot contain a whitespace character, " | |
| + f"a '{ApplicationProperties.__assignment_operator}' character, or " | |
| + f"a '{ApplicationProperties.__separator}' character when periods in keys are disabled." | |
| ) |
newdocs/src/changelog.md
Outdated
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): Minor grammar issue: "only manual set" should likely be "only manually set."
Suggest updating the parenthetical to: i.e. only manually set and "config" files are untyped, so only those are affected.
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 |
newdocs/src/changelog.md
Outdated
| <!-- pyml disable-next-line no-duplicate-heading--> | ||
| ### Fixed and Added | ||
|
|
||
| - Added `py.typed` file to ensure that type hints are consumed by package. |
There was a problem hiding this comment.
suggestion (typo): Minor grammar: add an article before "package."
Consider changing this to "consumed by the package" for more natural grammar.
Suggested change
| - Added `py.typed` file to ensure that type hints are consumed by package. | |
| - Added `py.typed` file to ensure that type hints are consumed by the package. |
newdocs/src/changelog.md
Outdated
| ### Fixed and Added | ||
|
|
||
| - Added `py.typed` file to ensure that type hints are consumed by package. | ||
| - Added `__all__` variable to `__init__.py` to resolve mypy error |
There was a problem hiding this comment.
suggestion (typo): Minor grammar: consider adding an article before "mypy error."
Change this to “to resolve a mypy error” for smoother grammar.
Suggested change
| - Added `__all__` variable to `__init__.py` to resolve mypy error | |
| - Added `__all__` variable to `__init__.py` to resolve a mypy error |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #321 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 10
Lines 657 661 +4
Branches 89 90 +1
=========================================
+ Hits 657 661 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
Update TOML loading to support keys containing periods when quoted, ensure explicit configuration files respect TOML section headers, and add tooling and documentation updates for the new release.
New Features:
Bug Fixes:
Enhancements:
Documentation: