improve configuration section merging of vectors

[vbargsten]

Currently, merging configuration sections with vectors results in an element-wise, recursive overwriting (or failure if overwrite=false). As a user, I would expect appending vector-like structures or a setting to select/define the behavior.
The recursive processing will also result in partial overwrites of elements of vectors within nested structures/maps, if the size differs.
I think it could be a useful feature for structuring more complex system configurations but needs improvement to be actually usable.

Example:
Consider applying the following configuration sections. If overwrite is enabled, you will end up with joint_names: ["finger_1", "finger_2"], or a failure otherwise. Instead, I would expect joint_names: ["joint_1", "joint_2", "finger_1", "finger_2"]. In addition, you get a filter "f2", with coeffs 5, 6, 3, 4.

--- name:manipulator merge:true
joint_names: ["joint_1", "joint_2"]
filters:
    - name: "f1"
      coeffs: [1, 2, 3, 4]

--- name:gripper merge:true
joint_names: ["finger_1", "finger_2"]

filters:
    - name: "f2"
      coeffs: [5, 6]

[doudou]

I understand the usefulness of what you propose, but I believe that it is basically the worst default behavior possible. Now, I also agree that the current behavior is very suboptimal - because very tricky. I guess that the rationale for it was to allow overriding some fields in an array without overriding the rest.

If I was to change this behavior, I would go for always overriding the complete array. This is the only policy that allows the user to completely define its configuration in every situation, and therefore is definitely the best default option.

Unfortunately, again, changing the behavior would very probably have strange effects on existing configuration systems. Having a graceful way to do these types of upgrade is on my todo list, but we all know how these things are sometimes.

Making this configurable, while always seemingly sounding interesting, is IMO not a good idea in this case. The flexibility you are seeking would bring the whole YAML loader closer to a programmable configuration language. YAML (or XML, JSON, ...) is really not meant for this, thus implementing it would IMO make the whole system overly complex, and for existing, but relatively few, use-cases. Doing this kind of flexible stuff is best done in a higher level layer. In Syskit systems, we end up having these kind of flexible configurations (which are few in the end) done within the Syskit system itself (e.g. in ruby).

[vbargsten]

I agree with you on the configurability.
However, I see the merge feature currently broken and can hardly imagine anyone using it this way.
Anyways, we are not talking about a default behavior, which is merge: false, afaik.
To summarize, I propose the following behavior:

• if merge: false and override: false
  -> sections can only define distinct properties
  -> otherwise error

• if merge: false and override: true
  -> override complete vectors, maps, scalars

• if merge: true and override: false
  -> merge maps (non-recursive), keys must be distinct
  -> append vectors
  -> otherwise error

• if merge: true and override: true
  -> override scalars
  -> merge maps (non-recursive), override values of existing keys
  -> append vectors

[doudou]

Anyways, we are not talking about a default behavior, which is merge: false, afaik.

It's not, unfortunately. Using merge:false in the config file is the default and by far the most widespread, that's true. But then, people apply lists of configuration sections which has the same effect (e.g. ['default', 'can0'])

What you just proposed makes. However, the use-case I feel is the most generally useful (at least, is the most useful for me :P) is not present:

• override scalars
• merge maps (non-recursive), override values of existing keys
• replace vectors

I'm really feeling un-helpful here. I agree that the current behavior must be changed but as I said we still don't have ways to cleanly change this type of behavior "cleanly" (e.g. through policy files the way Ruby on Rails or CMake do it), which IMO makes it impossible to change right now. And we don't have the time here to finally implement this mechanism.

My suggestion would be the following:

• implement the mechanism you propose and create a pull request, but hide the new behavior under a configuration flag that is local to orocos.rb
• this way, you can use the new behavior, and we can advertise its existence.
• once we do get the time to implement system-wide behavior policies, we'll control that flag using the policies.

Or if you feel like investing some time implementing the policy system ;-)