Skip to content

interpreter: Add values() method for dict #14432

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ePirat
Copy link
Contributor

@ePirat ePirat commented Apr 1, 2025

This makes it possible to retrieve a list of values of a dictionary:

dict = {
    'b': 'world',
    'a': 'hello',
}

dict.keys() # Returns ['a', 'b']

dict.values() # Returns ['hello', 'world']

Note that just like keys() returns the keys sorted, so will values() return them sorted by the corresponding keys to match between the two. I've updated the docs to clarify this. It is a bit unfortunate but I don't think it is ok to break behavior and make keys() not sort...

@ePirat ePirat requested a review from jpakkane as a code owner April 1, 2025 20:24
@ePirat ePirat force-pushed the epirat-add-dict-values-method branch from c22cdeb to cbd0dc9 Compare April 1, 2025 20:43
@ePirat
Copy link
Contributor Author

ePirat commented Apr 1, 2025

Updated to use list comprehension instead of map().

@ePirat ePirat force-pushed the epirat-add-dict-values-method branch 2 times, most recently from 595d9c1 to 5543b6d Compare April 1, 2025 21:34
Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

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

This all looks good to me, apart from the 1.7.2 -> 1.8.0 thing.

The question I have is, do we want to keep the sorting behavior. We have the following:

  1. the implementation of .keys() sorts
  2. we have no tests using .keys()
  3. we do not document anywhere that .keys() is sorted

I find the sorting behavior somewhat unexpected, and we don't document it or test it, but it may break something to change it. I also will need to do more git archeology to see where it came from

@bonzini
Copy link
Collaborator

bonzini commented Apr 2, 2025

I find the sorting behavior somewhat unexpected, and we don't document it or test it, but it may break something to change it. I also will need to do more git archeology to see where it came from

It came from #7900 (commit 4caa057), where changes to the iteration order caused changes in command lines. This in turn triggered unnecessary full rebuilds.

Anyhow, returning in values in matching order is a good idea.

@ePirat
Copy link
Contributor Author

ePirat commented Apr 2, 2025

@bonzini This should no longer be an issue given we guarantee insertion order and even document we do that. So ideally we should follow that for keys() and values() too, IMHO. Else it is quite confusing when iterating gives you a different order than these methods.

Maybe we should add a sorted kwarg to keys and values that defaults to false but can be "asc" or "desc"?

@bonzini
Copy link
Collaborator

bonzini commented Apr 2, 2025

That's fine for me, I'd even do only a boolean (false = insertion/true = ascending).

@eli-schwartz
Copy link
Member

eli-schwartz commented Apr 2, 2025

That original commit was made days before meson bumped the minimum python to 3.6, which had dictionaries that preserve order as an implementation detail -- and python 3.7 made that part of the language spec.

So at the time, sorting dictionaries could make a difference, but was effectively immediately made obsolete. Sorting is no longer required in order to have the intended effect of stable build.ninja file contents.

The other commits in that old PR are still relevant either way, since sets definitely do not have any order regardless of python version.

@dcbaker
Copy link
Member

dcbaker commented Apr 2, 2025

Given all the explanations, I'm not a bit concerned about removing the sorting by default. I'd be fine if we wanted to add an unsorted option that just says "they come back in some order", though I'm a bit concerned about requiring insertion order vs sorted order, that might have weird interactions with other implementations or with us doing any kind of internal fixups to a dictionary, which would violate the contract of an "insertion order" sort.

@ePirat
Copy link
Contributor Author

ePirat commented Apr 2, 2025

@dcbaker Not sure I understand your message? You say you are not concerned about removing the sorting but then you go on to say:

I'm a bit concerned about requiring insertion order

which sounds like directly contradicting what you just said. Can you clarify what you mean?

Also regarding internal fixups violating insertion order, we already guarantee and document dictionaries to maintain insertion order.

@eli-schwartz
Copy link
Member

https://mesonbuild.com/Reference-manual_elementary_dict.html#dict-dict

(since 0.62.0): Dictionary order is guaranteed to be insertion order.

This is funny since https://mesonbuild.com/Syntax.html#dictionaries still says:

Dictionaries are immutable and do not have a guaranteed order.


Anyway, I agree that we should have the same (documented in the RefMan) insertion order both using foreach and using .keys(). I am not entirely convinced that .values() is particularly useful to have, so I'm not going to approve this PR but I'm not going to argue against it either.

@dcbaker
Copy link
Member

dcbaker commented Apr 2, 2025

I guess as long as dictionaries are immutable it doesn't really matter (which they are), although I'd like to have options akin to the array one likes dict + dict, which might make this important?

@dcbaker
Copy link
Member

dcbaker commented Apr 2, 2025

@ePirat: At this point if we can fix the versions then I'm happy with this and would be fine to merge. I'd rather handle questions of different sort orders separately from values anyway

@ePirat
Copy link
Contributor Author

ePirat commented Apr 2, 2025

@dcbaker Ok, I will fix the versions then and we can do the discussion about sorting or not sorting in a follow up PR

ePirat added 3 commits April 2, 2025 23:23
Analogous to keys(), this returns the values in an array. It uses the
same sorting as keys(), else it would quite confusing to return values
in a different order than the corresponding keys.
@ePirat ePirat force-pushed the epirat-add-dict-values-method branch from 5543b6d to 2f6fdfb Compare April 2, 2025 21:24
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.

4 participants