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.

Sign up for GitHub

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

Jump to bottom

Simplify Channel constructors, change kwargs to positional args. #32818

Merged
merged 6 commits into from
Aug 11, 2019
Merged

Simplify Channel constructors, change kwargs to positional args. #32818

merged 6 commits into from
Aug 11, 2019

Conversation

NHDaly
Copy link
Member

@NHDaly NHDaly commented Aug 7, 2019

Oops, fixes this error, introduced in #30855:

WARNING: Method definition (::Type{Base.Channel{T}})(Function) where {T} in module Base at channels.jl:133 overwritten at channels.jl:136.

@NHDaly
Fix method overwrite warning for Channel constructors
d2c8796 
Oops, fixes this error, introduced in e891bca:
WARNING: Method definition (::Type{Base.Channel{T}})(Function) where {T} in module Base at channels.jl:133 overwritten at channels.jl:136.
NHDaly
NHDaly commented Aug 7, 2019
View reviewed changes
base/channels.jl Outdated Show resolved Hide resolved
@NHDaly NHDaly mentioned this pull request Aug 7, 2019
Merged
2 tasks
StefanKarpinski
StefanKarpinski reviewed Aug 7, 2019
View reviewed changes
base/channels.jl Outdated Show resolved Hide resolved
@NHDaly
Make Channel constructors interface more Julian
045ac09 
"Soft" deprecation of the old `Channel(f::Function; csize=0, ctype=Any)`
style in favor of the new `Channel{T=Any}(f::Function, size=0)`
interface.

Removed the old interface from the docs, so that it is not discoverable,
and included a `!!! compat` note that the new interface is new in Julia
1.3, and the old interface should not be used.
@NHDaly
Fix test/channels.jl to use newer Channel constructor
3a547d0
@NHDaly
Added new Channel constructor to news
b04fb84
@NHDaly
Copy link
Member Author

NHDaly commented Aug 9, 2019 •
edited
Loading

Okay, this should be good to go! :) Thanks again for the review.
PTAL

I think I will open one more PR for adding the spawn=false kwarg to the channels so that it can get a more thorough, independent review. Does that sound good? Thanks!

@JeffBezanson
Copy link
Sponsor Member

Looks good. I think this is missing a Channel(::Function, size) constructor (i.e. with no {T})?

@NHDaly
Copy link
Member Author

NHDaly commented Aug 9, 2019

Aye, so it is. :) Thanks good catch!

(I was about to fix it by pushing Channel(func::Function, args...; kwargs...) = Channel{Any}(func, args...; kwargs...), but then i realized i could just have a single T=Any constructor defined as Channel(args...; kwargs...) = Channel{Any}( args...; kwargs...) to cover both the function and non-function cases. Do you think that's valuable? Or is it less clear? Thanks!)

@JeffBezanson
Copy link
Sponsor Member

Either seems fine to me.

@NHDaly
Copy link
Member Author

NHDaly commented Aug 9, 2019 •
edited
Loading

EDIT: Ah, actually, i think there's an ambiguity with the ctype= constructors, so i may have to list the args explicitly anyway. :'( Oh maybe not actually.. okay i'll just pick one. thanks

@NHDaly
Copy link
Member Author

NHDaly commented Aug 9, 2019

Looks good. I think this is missing a Channel(::Function, size) constructor (i.e. with no {T})?

Okay thanks! :) Done!

@NHDaly NHDaly changed the title Fix method overwrite warning for Channel constructors Simply Channel constructors, change kwargs to positional args. Aug 9, 2019
@NHDaly NHDaly changed the title Simply Channel constructors, change kwargs to positional args. Simplify Channel constructors, change kwargs to positional args. Aug 9, 2019
JeffBezanson
JeffBezanson reviewed Aug 10, 2019
View reviewed changes
base/channels.jl
@@ -8,9 +8,9 @@ Representation of a channel passing objects of type `T`.
abstract type AbstractChannel{T} end

"""
Channel{T}(sz::Int=0)
Channel{T=Any}(size::Int=0)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Wow... maybe we should actually implement this syntax for defining constructors with default type parameter values? Seems quite possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow... maybe we should actually implement this syntax for defining constructors with default type parameter values? Seems quite possible.

:D Yeah that makes sense to me! It seems pretty readable, and has an intuitive meaning, i think.

@JeffBezanson JeffBezanson merged commit 0852465 into JuliaLang:master Aug 11, 2019
@NHDaly NHDaly deleted the Channel-constructors branch August 12, 2019 13:15
@NHDaly NHDaly mentioned this pull request Aug 12, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StefanKarpinski StefanKarpinski StefanKarpinski left review comments

@simonbyrne simonbyrne simonbyrne left review comments

@JeffBezanson JeffBezanson JeffBezanson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NHDaly @JeffBezanson @StefanKarpinski @simonbyrne