I think the for loop is clearer and easier to read, though it can be improved to use set instead of list, so we can remove seen_name_versions. what do you think?

If we don't need the the nvr_key_func in the change I previously commented, I'd say it's unnecessary to replace the lambda with a nested function.

I think the for loop is clearer and easier to read, though it can be improved to use set instead of list, so we can remove seen_name_versions. what do you think?

What the for-loop does is to group images by name-version and just get the top (latest) image from a name-version group. The grouping action is just what groupby does and using groupby can express the intention much straightforward. When someone reads the code, he/she does not need to translate the meaning of whole for-loop in mind. So, why not use a higher abstraction to make the code easier to read and understand?

I personally like the lambda approach more, it's just easier to understand in my opinion... but that's just my opinion.
Any other opinions?

Alternatively, you could just add a property to Containerimage to return the nvr?

@property
def nvr(self):
  return self['brew']['build']

Then you could even make sorted_by_nvr smart enough to detect this attr, and use it if available.

elif hasattr(item1, 'nvr') and hasattr(item2, 'nvr'):
  nvr1 = item1.nvr
  nvr2 = item2.nvr

@lucarval Thanks for the idea. PR is updated accordingly.

item.nvr ? :)

This function doesn't actually group items. It returns the name and version for a given item. Can we rename it accordingly? e.g. _get_name_and_version ?

Thanks @lucarval PTAL.

BTW, there are still many image["brew"]["build"] in the code. I'll replace them with image.nvr after this PR is merged.

Looks good :) +1

:thumbsup: Thanks for refactoring this piece of the code!

Commit 336eb7ab fixes this pull-request