Patching nested YAML map values incorrectly
lotem opened this issue · 7 comments
In *.custom.yaml
, values from the immediate children key-value pairs under the node patch:
will substitute values with the same key-path from the YAML document.
It's important to specify in the patch a key-path separated with /
for values in a nested YAML map.
For example, the following code in RimeConfigController.m:
[_squirrelConfig patchValue:_colorTheme forKeyPath:@"style.color_scheme" error:&error];
generates an incorrect patch
patch:
style:
color_scheme: blah
which should be:
patch:
"style/color_scheme": blah
The problem with the former patch is, other children of the node style
would be lost in the patched file, since the YAML value to be patched is the entire map (ROOT)/style
.
However, the former patch is acceptable, and reasonable to Rime, in case the user is intentionally replacing the whole map.
It is really weird for me that the former form will patch the whole "style" but not "style.color_scheme". Will look for a fix ASAP.
BTW any chance that Rime would support both the two forms you listed above in the exactly same way? After all they are identical in the semantical POV.
Yes, the two forms are both supported, but the meanings are different.
In some cases, it's necessary for the patch to be able to replace an entire map:
# default.custom.yaml
patch:
punctuator/half_shape: {}
This effectively clears predefined punctuation table.
Once you patch a list, the list is not appended with new items in the patch but is replaced with them; the same goes with a map. Otherwise, there will be no way to rewrite a map entirely.
Not requiring changes in Rime lib but the following rules is my pre-assumption when implemented patchValue (seems not the one it really works with though -_-):
- The following form patch the value for key path
style.horizontal
and not the wholestyle
dictionary:
patch:
style:
horizontal: true
- The following form acts exactly the same as the previous one, patch the value for key path
style.horizontal
:
patch:
"style/horizontal": true
- If we want to clear a dictionary or a array we can use the following form, in very explicit way:
patch:
style: {}
app_options: []
After all your method would also work fine and I will change my patchValue method to support it ASAP.
Unluckily, in YAML syntax,
patch:
style: { horizontal: true }
is just an equivalent representation for
patch:
style:
horizontal: true
and, if empty maps and empty lists mean nothing significant but absence,
patch:
style: {}
app_options: []
does the same job with
patch:
style:
app_options:
where in the latter form, null values are implied by leaving nothing after the key.
On the other hand, finding leaf nodes recursively in the tree introduces unnecessary complexity;
moreover, allowing an alternative representation also make it harder to find duplicate key-paths in the patch.
That's why I insisted on using a flat map of key-path - value.
Though it looked a little awkward to human eyes at first, it keeps the logic simple and consistent.
I've got your point. I will make the 0.4 release focused on this issue in a couple of days. Thanks for the explanation.