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

IndexValue needs better docs #25675

Open
kshyatt opened this issue Jan 21, 2018 · 11 comments
Open

IndexValue needs better docs #25675

kshyatt opened this issue Jan 21, 2018 · 11 comments
Labels
collections Data structures holding multiple items, e.g. sets docs This change adds or pertains to documentation

Comments

@kshyatt
Copy link
Contributor

kshyatt commented Jan 21, 2018

Spent some frustrating time yesterday in Coverage.jl land trying to figure out what this struct is, what it does, and (most importantly) why it exists. I'm sure it's lovely and solves many problems! But a bit more of an explanation of where/why to use it (as opposed to a regular Dict?) and how to do some basic ops with it (can I push! to it? how do I convert back and forth to other AbstractDict types?) would be great.

@kshyatt kshyatt added docs This change adds or pertains to documentation collections Data structures holding multiple items, e.g. sets labels Jan 21, 2018
@JeffBezanson
Copy link
Sponsor Member

I believe this was intended, at least originally, as an implementation detail of the pairs function.

@ararslan
Copy link
Member

ararslan commented Jan 23, 2018

I think it was implemented to avoid breaking (key, value) iteration over keyword arguments. When keyword arguments became NamedTuples (#24795), iterating over keyword arguments no longer included the name of the argument, since NamedTuples iterate values rather than pairs, which was breaking. #25290, which sought to undo the breakage, changed the type of keyword arguments from NamedTuple to Iterators.IndexValue, which iterates pairs rather than values.

While IndexValue isn't necessarily part of the public API, it would be good to know what can or cannot be done with it, since it's now the type of keyword arguments. Currently the only documentation is: "Transforms an indexable container into an Dictionary-view of the same data. Modifying the key-space of the underlying data may invalidate this object." This is not particularly informative, IMO.

As an aside, I would also support re-breaking iteration over keyword arguments by making them plain ol' NamedTuples...

@yurivish
Copy link
Contributor

I'd absolutely support re-breaking iteration over keyword arguments by restoring them to NamedTuples.

@StefanKarpinski
Copy link
Sponsor Member

100% agree, seems better to just rip that bandaid off now instead of letting it live in a halfway state.

@ararslan
Copy link
Member

See #25711. Regardless of whether this is changed, IndexValue could do with some better docs.

@JeffBezanson
Copy link
Sponsor Member

it was implemented to avoid breaking (key, value) iteration over keyword arguments

Not strictly true; I think it was introduced to implement pairs for arrays.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jan 24, 2018

Why would we ever want them to iterate without returning pairs? Seems way more annoying to default to doing the wrong behavior. In v1.0, I expect the NT type will lose it's priviledged-hack status and become just another normal type. Thus we can always have keywords behave exactly how we want rather than just documenting the gotchas (like iteration being broken).

@JeffBezanson
Copy link
Sponsor Member

It's really not helpful to keep describing the behavior you don't prefer as "wrong" and "broken". To a python programmer, iterating pairs is "broken" since they're used to iterating keys. Or, as has been pointed out several times, if you call a function like maximum on a dict then iterating pairs is also "wrong". Yes, if you expect it to iterate pairs then iterating values is "wrong". But none of these uses of the word match my definition of "wrong".

the NT type will lose it's priviledged-hack status

...and the downtrodden and oppressed will rise, and freedom will ring throughout the land.

and become just another normal type. Thus we can always have keywords behave exactly how we want

This seems like a non-sequitur to me. Sure, we can change named tuples to be implemented with dot overloading. But we can have keywords behave how we want regardless of that. If we want keyword arguments to iterate pairs, we can keep things the way they are, or we can define a new type that's the same as NamedTuple except for iteration behavior. But people like consistency, and it's not ideal to introduce a new kind of thing for each use case instead of re-using functionality.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jan 24, 2018

if you call a function like maximum

Seems to work, and return the largest element as expected: maximum(Dict(1 => 2, 3 => 0)) ==> 3 => 0. I realize that there has been some debate about whether this is inconsistent with indmax and findmax.

@fredrikekre
Copy link
Member

Fixed by #25764 (?)

@ararslan
Copy link
Member

ararslan commented Feb 5, 2018

Not really, since that PR didn't improve the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Data structures holding multiple items, e.g. sets docs This change adds or pertains to documentation
Projects
None yet
Development

No branches or pull requests

7 participants