teemtee / fmf

Flexible Metadata Format
GNU General Public License v2.0
22 stars 28 forks source link

Inconsistent order of operations #232

Open LecrisUT opened 5 months ago

LecrisUT commented 5 months ago

Example:

my_var:
  - one
  - two

/overwrite:
  my_var-: [ two, three ]
  my_var+: [ three, four ]

This one gives my_var: one and four which would be inconsistent based on the order of definition. This might not be that crucial, but at least it should be documented that the keys are ordered before being parsed by fmf, thus the order is ...

Are there tests at least covering the operation order based on inheritance (inherited operations occur first, e.g. if my_var-: was one level below, three should be in the list) and adjust (after base variable operations)?

lukaszachy commented 4 months ago

We need to document - implementation sorts keys - https://github.com/teemtee/fmf/blob/29190c78e4065adf98cbb3ccf747b01973d761e2/fmf/base.py#L222C1-L222C50

LecrisUT commented 4 months ago

Oh, thanks for the info. Than the only other part of the issue is if there are tests that cover the plain order, the order with inheritance and the order with adjust

lukaszachy commented 4 months ago

Ack, coverage should be added as mistakes in refactor might happen.

psss commented 4 months ago

We need to document - implementation sorts keys - https://github.com/teemtee/fmf/blob/29190c78e4065adf98cbb3ccf747b01973d761e2/fmf/base.py#L222C1-L222C50

Hm, just pondering, is that sorted() call really intentional? I looked through the history and it appears in 51bf8defe076afa8fc8cea39c138a9f273459367 which is the very first fmf commit ;-) I'm not really sure we need to keep it. On the other hand, if removed (which could fix the problem reported here), can we rely on the order of the dict items?

psss commented 4 months ago

Decision from the hacking session: Respect the order of the adjustments and drop the sorted() from the code. Document the behavior.

LecrisUT commented 4 months ago

Could break backwards compatibility, but I think this is quite a niche usage, we could just scrape the public .fmf files and announce the change there.

Edit: as far as I see we're safe: https://sourcegraph.com/search?q=context:global+file:.*%5C.fmf+%28%5Cw%2B%29%28-%7C%5C%2B%3C%29:&patternType=regexp&sm=0