fix json.assign_to_path with non nested path#13245
Conversation
Test Results (amd64) - Integration, Bootstrap 5 files ± 0 5 suites ±0 2h 39m 14s ⏱️ +45s Results for commit 478d6a2. ± Comparison against base commit e0a4a18. This pull request removes 1 and adds 57 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 2h 0m 6s ⏱️ +24s Results for commit 478d6a2. ± Comparison against base commit e0a4a18. This pull request removes 1 and adds 57 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
eceed06 to
213dd95
Compare
simonrw
left a comment
There was a problem hiding this comment.
This change seems fine for the use case we want to use it for, however I think this function in general could do with work (outside the scope of this change).
For example, I expected
assign_to_path({}, "a.b.c", "foo") == {"a": {"b": {"c": "foo"}}}however it equals {"a.b": {"c": "foo"}}.
Perhaps consider refactoring the PR that requires this change to not use this function?
| @@ -171,6 +171,11 @@ def extract_jsonpath(value, path): | |||
|
|
|||
| def assign_to_path(target, path: str, value, delimiter: str = "."): | |||
There was a problem hiding this comment.
suggestion: really minor but going with the attitude of "leave the codebase in a better shape", perhaps you could add a docstring to explain what this function is supposed to do?
There was a problem hiding this comment.
I agree, some typing would be lovely as well!
There was a problem hiding this comment.
I can add the docstring as well.
I think @simonrw discovered a new bug in this implementation though... since I would assume the intended behavior would be to return and the way it works when the delimiter is "/". All we need to do is to pass in the delimiter to extract_from_jsonpointer_path.
All of the use cases I could see in the code are using "/" as a delimiter, so it shouldn't break any existing behavior.
Good catch @simonrw
dfangl
left a comment
There was a problem hiding this comment.
I agree with Simon, however the PR is fine for me otherwise.
| @@ -171,6 +171,11 @@ def extract_jsonpath(value, path): | |||
|
|
|||
| def assign_to_path(target, path: str, value, delimiter: str = "."): | |||
There was a problem hiding this comment.
I agree, some typing would be lovely as well!
Motivation
This pr aims to fix an issue where attempting to assign a non-nested path, would result in a blank string key added to the dict. For example
assign_to_path({}, "key", "value")would return{"": {"key": "value"}}. Instead of requiring consumer of the util to strip and split to verify if the path is indeed nested, it seems more appropriate to return the expected value{"key", "value"}.part of UNC-17
Changes
assign_to_pathto return early if the path is not nested.