Skip to content
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

Patching nested YAML map values incorrectly #5

Open
lotem opened this issue Feb 15, 2013 · 7 comments
Open

Patching nested YAML map values incorrectly #5

lotem opened this issue Feb 15, 2013 · 7 comments
Assignees
Labels
Milestone

Comments

@lotem
Copy link

lotem commented Feb 15, 2013

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.

@lotem
Copy link
Author

lotem commented Feb 15, 2013

However, the former patch is acceptable, and reasonable to Rime, in case the user is intentionally replacing the whole map.

@neolee
Copy link
Owner

neolee commented Feb 15, 2013

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.

@neolee
Copy link
Owner

neolee commented Feb 15, 2013

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.

@lotem
Copy link
Author

lotem commented Feb 16, 2013

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.

@neolee
Copy link
Owner

neolee commented Feb 16, 2013

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 whole style 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.

@ghost ghost assigned neolee Feb 16, 2013
@lotem
Copy link
Author

lotem commented Feb 16, 2013

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.

@neolee
Copy link
Owner

neolee commented Feb 16, 2013

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants