RFC: define a single otk.op.join

[mvo5]

I wonder if we can simply avoid having the user care about the type when doing a join operation. It seems mostly a burden to the user and we need to check during compilation of the otk file anyway so maybe we can just leave it out?

P.S. Having an example use-case for both would be really good, I'm wonder if we only should add things to the spec once we actually use it in the example/* tree ?

[kingsleyzissou]

Having an example use-case for both would be really good

This was a question I've been asking and was going to put it down, but it seems you have beaten me to asking it :)

[supakeen]

My reasoning for this was that having separate operations per type allows us to have separate arguments per type. For a map erroring on duplicates (or overwriting them) makes sense. For a sequence less so.

[mvo5]

My reasoning for this was that having separate operations per type allows us to have separate arguments per type. For a map erroring on duplicates (or overwriting them) makes sense. For a sequence less so.

Thanks! I'm not sure I follow - if otk.op.join would be used on two maps with duplicates it would still error. If it is used on two sequences with duplicates it would not. Mixing the two is not allowed. What am I missing?

[supakeen]

My reasoning for this was that having separate operations per type allows us to have separate arguments per type. For a map erroring on duplicates (or overwriting them) makes sense. For a sequence less so.

Thanks! I'm not sure I follow - if otk.op.join would be used on two maps with duplicates it would still error. If it is used on two sequences with duplicates it would not. Mixing the two is not allowed. What am I missing?

Wanting to define the behavior, for example (don't pin me on this one ;)).

otk.op.map.join:
  duplicates: overwrite
  values:
    - key:
        one
    - key:
        two

The duplicates key would not do anything on the otk.op.seq.join I feel?

[mvo5]

My reasoning for this was that having separate operations per type allows us to have separate arguments per type. For a map erroring on duplicates (or overwriting them) makes sense. For a sequence less so.

Thanks! I'm not sure I follow - if otk.op.join would be used on two maps with duplicates it would still error. If it is used on two sequences with duplicates it would not. Mixing the two is not allowed. What am I missing?

Wanting to define the behavior, for example (don't pin me on this one ;)).

otk.op.map.join:
  duplicates: overwrite
  values:
    - key:
        one
    - key:
        two

The duplicates key would not do anything on the otk.op.seq.join I feel?

Thank you! That is interesting (and the context I was looking for!). We could start with what popular languages like python are doing:

1. duplicated keys in a map get replaced by "later" joins (essentially meaning overwrite)
2. seq are just joined

I like the idea of the users having a say here though, so once the above is not good enough anymore, we could add rules like

duplicates: [error, overwrite]

and then we could decide to error if "overwrite" is used on sequences. Or we could also "bend" the definition and "de-duplicate/replace/overwrite" keys in a seqence if later sequences also define it (not sure I like it or not but it's an option :)

[mvo5]

I fixed the conflicts, sorry that this took me a bit

[supakeen]

This does still need the implementation change at: https://github.com/osbuild/otk/blob/main/src/otk/directive.py#L107-L154 and https://github.com/osbuild/otk/blob/main/src/otk/transform.py#L51-L52 :)

[kingsleyzissou]

Duplicate keys in maps are considered an error.

It might be nice to have a test for this (unless there is one already and I missed it)

[supakeen]

List wasn't imported from typing.

[supakeen]

Duplicate keys in maps are considered an error.

It might be nice to have a test for this (unless there is one already and I missed it)

There's no test for it (yet) and the code doesn't actually check this:

otk/src/otk/directive.py

Line 148 in 9ead440

for value in values:

It should straightforward to add, or reconsider if it should be an error.

[kingsleyzissou]

It should straightforward to add, or reconsider if it should be an error.

Yeah doesn't have to be for this PR :)

[supakeen]

@mvo5 you imported List in the wrong module ;)

[mvo5]

@mvo5 you imported List in the wrong module ;)

Silly me, sorry for that! Fwiw, make type gives me a bunch of errors so I did not see this one but it's not really important at this point (the wrong List import is hopefully fixed now).