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

return pairs instead of tuples when iterating Associatives #12327

Merged
merged 2 commits into from
Jul 27, 2015

Conversation

JeffBezanson
Copy link
Member

fixes #12261

function in(p::Tuple{Any,Any}, a::Associative)
function in(p, a::Associative)
if !isa(p,Pair)
error("Associative collections only contain Pairs; look for e.g. A=>B instead")
Copy link
Member

Choose a reason for hiding this comment

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

Would it be cleaner to use dispatch here?

Should this be a TypeError?

Copy link
Member

Choose a reason for hiding this comment

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

The descriptive error message seems fine to me, though I'd like it to also point people to keys and values function, since I think if people are in this situation they are most likely looking for one or the other.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could use dispatch, but there is a fallback definition of in for all types that will unhelpfully just return false in this case. This isn't really a type error, since it's perfectly valid to ask whether some random thing is in a collection. This is kind of a "fuzzy" error just meant to be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Keno yes good idea.

Copy link
Member

Choose a reason for hiding this comment

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

@JeffBezanson I thought that function in(p, a::Associative) and function in(p::Pair, a::Associative) would fallback correctly/equivalently? (or specific error for function in(p::Tuple{Any,Any}, a::Associative)).

+1 to directing towards A in keys(a) and B in values(a)

@JeffBezanson
Copy link
Member Author

Also note that before this change collect(ENV) was broken. It returned arrays instead of tuples (leading to a no method error in convert).

@ivarne
Copy link
Member

ivarne commented Jul 27, 2015

@JeffBezanson @tkelman Seems like that makes parts of this PR eligible for backport to the release-0.3 branch.

@JeffBezanson
Copy link
Member Author

release-0.3 would need a different fix; it doesn't have Pairs.

@ivarne
Copy link
Member

ivarne commented Jul 27, 2015

Yes, definitely. Tuples (not Pair) for 0.3

JeffBezanson added a commit that referenced this pull request Jul 27, 2015
return pairs instead of tuples when iterating Associatives
@JeffBezanson JeffBezanson merged commit 2057e07 into master Jul 27, 2015
@StefanKarpinski
Copy link
Member

We should probably have test for the ENV thing then, no?

@JeffBezanson
Copy link
Member Author

Yes.

@tkelman tkelman deleted the jb/returnpairs branch July 28, 2015 00:21
@tkelman
Copy link
Contributor

tkelman commented Jul 28, 2015

If no one has tried to collect(ENV) on release-0.3 yet, then I'm not going to be in a rush to backport anything. I'm not sure if we'll really need a 0.3.12 release, unless people want to go hunting for other simple bugfixes.

tkelman referenced this pull request Jul 29, 2015
- Fix some more 0.4 deprecations
- Add support for emoji_symbols
- Handle characters that do not have assigned Unicode names
jiahao added a commit that referenced this pull request Jul 29, 2015
Creating a sorted list of (key, value) pairs now requires specifying
sort by=first, since iterating through a dictionary now returns Pairs
rather than Tuples, and Pairs do not have a defined lexical order.

Ref: cc61506 #12327
JeffBezanson added a commit that referenced this pull request Jul 30, 2015
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.

Should iteration over Associatives return Pairs
6 participants