-
Notifications
You must be signed in to change notification settings - Fork 84
feat: Add flattening_separator as an additional flattening config
#3415
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: Add flattening_separator as an additional flattening config
#3415
Conversation
Reviewer's GuideAdds support for a configurable flattening_separator in flattening options and extends flattening helpers and tests to honor a custom separator instead of the hard-coded '__'. Class diagram for updated flattening configuration supportclassDiagram
class PluginFlatteningConfig {
<<TypedDict>>
bool flattening_enabled
int flattening_max_depth
int flattening_max_key_length
str flattening_separator
}
class FlatteningOptions {
bool enabled
int max_depth
int max_key_length
str separator
+from_dict(cls, data: PluginFlatteningConfig) FlatteningOptions
}
PluginFlatteningConfig --> FlatteningOptions : used_by_from_dict
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Consider marking
flattening_separatoras optional/NotRequired inPluginFlatteningConfigso the type signature matches existing usage where this key is not always present. - The new custom-separator tests duplicate some of the existing schema/record structures; extracting a shared fixture or helper for these nested structures would reduce repetition and make future changes easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider marking `flattening_separator` as optional/NotRequired in `PluginFlatteningConfig` so the type signature matches existing usage where this key is not always present.
- The new custom-separator tests duplicate some of the existing schema/record structures; extracting a shared fixture or helper for these nested structures would reduce repetition and make future changes easier.
## Individual Comments
### Comment 1
<location> `tests/core/test_flattening.py:349-358` </location>
<code_context>
+def test_flatten_schema_with_custom_separator():
</code_context>
<issue_to_address>
**suggestion (testing):** Add an end-to-end test that wires `flattening_separator` from config through to `flatten_schema`/`flatten_record`.
Current tests show that `flatten_schema`/`flatten_record` respect a custom `separator` and that config is parsed into `FlatteningOptions`, but they don’t cover the full flow from plugin config through `get_flattening_options` to the flattening helpers. Please add an integration-style test that calls `get_flattening_options` with a custom `flattening_separator`, then uses the returned `FlatteningOptions` with `flatten_schema` and/or `flatten_record`, asserting that the flattened keys use the configured separator.
Suggested implementation:
```python
def test_flatten_schema_with_custom_separator():
"""End-to-end test that flattening_separator in config flows through to flattening helpers."""
# Build a config that specifies a custom flattening separator
config = {
"flattening_enabled": True,
"flattening_max_depth": 10,
"flattening_separator": "__",
}
# Get options from config
options = get_flattening_options(config)
# Schema with nested object to verify key flattening
schema = {
"type": "object",
"properties": {
"key_1": {"type": "string"},
"key_2": {
"type": "object",
"properties": {
"key_3": {"type": "string"},
},
},
},
}
# Record matching the schema
record = {"key_1": "value1", "key_2": {"key_3": "value3"}}
# When flattening using the options derived from config
flattened_schema = flatten_schema(schema, options=options)
flattened_record = flatten_record(record, schema=schema, options=options)
# Then the configured separator is used in the flattened keys
assert "key_2__key_3" in flattened_schema["properties"]
assert "key_2__key_3" in flattened_record
assert flattened_record["key_2__key_3"] == "value3"
```
This change assumes the following existing interfaces, which are consistent with the rest of the tests in this module:
1. `get_flattening_options(config: dict) -> FlatteningOptions`
2. `flatten_schema(schema: dict, options: FlatteningOptions) -> dict`
3. `flatten_record(record: dict, schema: dict, options: FlatteningOptions) -> dict`
If your actual function signatures differ (for example, if `options` is a positional argument or named differently), adjust the calls in the new test accordingly. Also ensure that `get_flattening_options`, `flatten_schema`, and `flatten_record` are imported at the top of `tests/core/test_flattening.py` if they are not already.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3415 +/- ##
=======================================
Coverage 94.13% 94.14%
=======================================
Files 69 69
Lines 5782 5785 +3
Branches 716 717 +1
=======================================
+ Hits 5443 5446 +3
Misses 236 236
Partials 103 103
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
flattening_separator as an additional flattening config
Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
Description
This PR allows users to set the
flattening_separatorconfig along side the other flattening options. It is used override the default separator of "__".Related Issues
Closes #3401
Summary by Sourcery
Add support for configuring a custom flattening separator and validate it in flattening utilities.
New Features:
Tests: