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

Add broadcastable method for Enum #30670

Closed
wants to merge 4 commits into from

Conversation

@AzamatB
Copy link
Contributor

commented Jan 9, 2019

Fixes #30669.

@@ -606,6 +606,7 @@ Base.RefValue{String}("hello")
```
"""
broadcastable(x::Union{Symbol,AbstractString,Function,UndefInitializer,Nothing,RoundingMode,Missing,Val}) = Ref(x)

This comment has been minimized.

Copy link
@AzamatB

AzamatB Jan 9, 2019

Author Contributor

Should we consolidate these definitions

broadcastable(x::Union{Symbol,AbstractString,Function,UndefInitializer,Nothing,RoundingMode,Missing,Val}) = Ref(x)
broadcastable(x::Enum) = Ref(x)
broadcastable(x::Ptr) = Ref(x)
broadcastable(r::Regex) = Ref(r)

as one into

broadcastable(x::Union{Symbol,AbstractString,Function,UndefInitializer,Nothing,RoundingMode,Missing,Val,Ptr,Regex,Enum}) = Ref(x)

?

This comment has been minimized.

Copy link
@stevengj

stevengj Jan 10, 2019

Member

The Enum definition can't go here, since Enums.jl is loaded after this file. So you need to put Base.broadcastable(x::Enum) = Ref(x) into Enums.jl instead.

We could additionally consolidate this code somewhat as you say, but for simplicity you could leave it as-is in this PR.

@@ -163,3 +163,6 @@ end
haggis = 4
end
@test Int(haggis) == 4

# test scalar behavior in broadcast
@test (@enum Fruit apple; v = Vector{Fruit}(undef, 3); v .= apple; v) == [apple, apple, apple]

This comment has been minimized.

Copy link
@stevengj

stevengj Jan 10, 2019

Member

The Fruit enum is already defined in this test file. So you can just do

@test (Vector{Fruit}(undef, 3) .= apple) == [apple, apple, apple]

(Note that there is no need to define the v variable, and the return value of .= is the lhs.)

@stevengj stevengj removed the needs tests label Jan 10, 2019

@AzamatB

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

Thanks for the explanations! This is one of my first PRs :)
Superseded by #30675.

@AzamatB AzamatB closed this Jan 10, 2019

@stevengj

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

In the future you should update an existing PR rather than creating a new one, so as not to break up the discussion.

@AzamatB

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

Thanks for letting me know. I would've done so but was not able to add commits here anymore. I suppose it is because I've made this PR without forking... Maybe it is possible, but I'm not sufficiently good at git to achieve this. I did the new PR properly forking the master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.