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

RFC: Set to Set conversions #18727

Merged
merged 1 commit into from Sep 30, 2016
Merged

RFC: Set to Set conversions #18727

merged 1 commit into from Sep 30, 2016

Conversation

ghost
Copy link

@ghost ghost commented Sep 29, 2016

As a major set fan I found it somewhat odd that convert(Vector{String}, split("foo bar baz qux")) works, but the same logic for sets, convert(Set{String}, Set(split("foo bar baz qux"))), doesn't. This is a patch based on my own little hack that I introduced in a module to get around the aforementioned issue. Feedback is as always most welcome.

@ghost
Copy link
Author

ghost commented Sep 29, 2016

Test failure on x86_64 appears unrelated, should I force push to trigger a re-run?

@vchuravy
Copy link
Member

@@ -193,3 +193,6 @@ function hash(s::Set, h::UInt)
end
return h
end

convert{T}(::Type{Set{T}}, s::Set{T}) = s
convert{T,S}(::Type{Set{T}}, x::Set{S}) = Set{T}(convert(T, e) for e in x)
Copy link
Member

Choose a reason for hiding this comment

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

I believe just Set{T}(x) would work.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, I really should have realised that one on my own.

@JeffBezanson
Copy link
Member

👍 Really surprising we didn't have this already!

@ghost
Copy link
Author

ghost commented Sep 29, 2016

Improved version pushed. I also think I will submit a similar PR for Pair.

iset = Set([17, 4711])
@test convert(Set{Float64}, iset) == iset
fset = Set([17.0, 4711.0])
@test convert(Set{Int}, fset) == fset
Copy link
Contributor

Choose a reason for hiding this comment

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

test the element types of the result if possible

Copy link
Author

Choose a reason for hiding this comment

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

In a hurry this morning, so I only dug through the manual briefly and pushed the cleanest (still dirty) way I could think of doing this. More julian suggestions are most welcome.

@JeffBezanson JeffBezanson merged commit 862fb74 into JuliaLang:master Sep 30, 2016
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.

3 participants