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

Add support for merging list with dict #256

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kkaarreell
Copy link

No description provided.

@kkaarreell
Copy link
Author

it is questionable if we really need a new operator. Can't we simply stick to +?

@kkaarreell
Copy link
Author

kkaarreell commented Nov 7, 2024

$ cat main.fmf 
discover:
    - how: fmf
      url: https://github.com/project1
    - how: fmf
      url: https://github.com/project2

/basic:
    summary: Quick set of basic functionality tests
    discover+*:
        filter: "tier:1"

/features:
    summary: Detailed tests for individual features
    discover+*:
        filter: "tier:2"
$ fmf show
/basic
discover: [{'filter': 'tier:1', 'how': 'fmf', 'url': 'https://github.com/project1'},
 {'filter': 'tier:1', 'how': 'fmf', 'url': 'https://github.com/project2'}]
summary: Quick set of basic functionality tests

/features
discover: [{'filter': 'tier:2', 'how': 'fmf', 'url': 'https://github.com/project1'},
 {'filter': 'tier:2', 'how': 'fmf', 'url': 'https://github.com/project2'}]
summary: Detailed tests for individual features

@LecrisUT
Copy link
Contributor

LecrisUT commented Nov 7, 2024

Just a thought, if we go with the * operator, I think it would be better for it to be *+

@kkaarreell
Copy link
Author

So, after reading through the original issue it seems there are more people favoring using the + operator.
With that in mind, I have pushed another commit which modified the code so that only + is used.

I like this option more (if you want to compare both variants see the 1st commit only).

Moreover, now the following use case seems to be handled as well:

discover:
    how: fmf
    prune: True
    adjust-tests:
        - duration+: '*3'
          when: hw == slow

/path:
  discover+:
    - name: upstream
      url: https://some.url
    - name: downstream
      url: https://other.url
$ fmf show
/path
discover: [{'adjust-tests': [{'duration+': '*3', 'when': 'hw == slow'}],
  'how': 'fmf',
  'name': 'upstream',
  'prune': True,
  'url': 'https://some.url'},
 {'adjust-tests': [{'duration+': '*3', 'when': 'hw == slow'}],
  'how': 'fmf',
  'name': 'downstream',
  'prune': True,
  'url': 'https://other.url'}]

fmf/base.py Show resolved Hide resolved
if not isinstance(list_item, dict):
"MergeError: Item '{0}' in {1} must be a dictionary.".format(
list_item, self.name)
self._merge_special(list_item, data[key])
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, what's happening with those original something+ attributes? I mean, we do the update of the respective something attribute but what happens with something+. Does fmf care about it? Does it matter if we modify it? I am asking because this code is in fact modifying the something+ element as well.

@kkaarreell kkaarreell changed the title Add support for +* operation Add support for merging list with dict Nov 19, 2024
@kkaarreell kkaarreell force-pushed the ks_merge_star branch 2 times, most recently from 9fc47a2 to 1bedbb5 Compare November 19, 2024 12:56
@kkaarreell
Copy link
Author

@psss Hi, I do not plan any further updates unless requested. Please review.

@kkaarreell
Copy link
Author

/packit retest-failed

@kkaarreell
Copy link
Author

@psss Hi, could you please check failing tests? It doesn't seem related, rather like a change in Fedora distro that is not reflected in tests. Thank you.

@kkaarreell
Copy link
Author

Tests failures doesn't seem to be related. Same failures were present even when I have reverted my commits.

@psss psss added this to the 1.5 milestone Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants