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

Fix show() error while calling UDPSocket() in REPL. #14325

Merged
merged 2 commits into from
Dec 21, 2015
Merged

Fix show() error while calling UDPSocket() in REPL. #14325

merged 2 commits into from
Dec 21, 2015

Conversation

abhijithanilkumar
Copy link
Contributor

Calling UDPSocket() displays this now.

julia> UDPSocket()
UDPSocket(init)

Fixes #13711

@abhijithanilkumar abhijithanilkumar changed the title Fix show() error while calling UDPSocket() in REPL. Fix show() error while calling UDPSocket() in REPL. fixes #13711 Dec 8, 2015
@abhijithanilkumar abhijithanilkumar changed the title Fix show() error while calling UDPSocket() in REPL. fixes #13711 Fix show() error while calling UDPSocket() in REPL. Dec 8, 2015
@tkelman tkelman added the needs tests Unit tests are required for this change label Dec 8, 2015
@abhijithanilkumar
Copy link
Contributor Author

@tkelman What tests should be implemented here?

@kshyatt
Copy link
Contributor

kshyatt commented Dec 8, 2015

@abhijithanilkumar tests for show can be seen here.

@abhijithanilkumar
Copy link
Contributor Author

@kshyatt Is the displayed message fine? Or should it be changed to something else?

@tkelman
Copy link
Contributor

tkelman commented Dec 8, 2015

This is good, though there's an underlying issue that the show method that was previously being called for the LibuvStream abstract type assumes the presence of a buffer field. That's apparently a bad assumption, question is whether there are counterexamples other than UDPSocket?

@abhijithanilkumar
Copy link
Contributor Author

@tkelman TCPSocket and UDPSocket uses LibuvStream. TCPSocket works fine.

@kshyatt
Copy link
Contributor

kshyatt commented Dec 8, 2015

I think the message is fine. @tkelman I grepped for <: LibuvStream and every other subtype has a buffer member.

@tkelman
Copy link
Contributor

tkelman commented Dec 8, 2015

I'm not sure whether that type is meant to be subtyped by packages. If not, maybe the existing show method should be made more specific to only apply to TCPSocket.

@kshyatt
Copy link
Contributor

kshyatt commented Dec 8, 2015

The existing show method also works for TTY AFAICT.

@tkelman
Copy link
Contributor

tkelman commented Dec 8, 2015

Are there other methods on the abstract LibuvStream that make similar type member assumptions? show might not be the only thing broken.

@rohitvarkey
Copy link
Contributor

@tkelman @abhijithanilkumar Seems like there are a few methods which are broken due to this assumption. Few examples. (This is on 0.4.1)

julia> nb_available(s)
ERROR: type UDPSocket has no field buffer
 in nb_available at stream.jl:254

julia> isreadable(s);
ERROR: type UDPSocket has no field buffer
 in isreadable at stream.jl:243

julia> isopen(s);
ERROR: type UDPSocket has no field buffer
 in print at strings/io.jl:8
 in print_to_string at strings/io.jl:36
 in isopen at stream.jl:317

@rohitvarkey
Copy link
Contributor

I also found a duplicate method for nb_available.

nb_available(s::LibuvStream) = nb_available(s.buffer)

nb_available(stream::LibuvStream) = nb_available(stream.buffer)

@abhijithanilkumar
Copy link
Contributor Author

@tkelman What has to be done here?

@tkelman
Copy link
Contributor

tkelman commented Dec 11, 2015

needs to add tests of this method

@abhijithanilkumar
Copy link
Contributor Author

@tkelman I'm a newbie to Julia. Where should I add the tests? 😅 @kshyatt I didn't really understand show tests you posted. I thought I was making a call to an already existing show method. Please correct me if I'm wrong.

@tkelman
Copy link
Contributor

tkelman commented Dec 14, 2015

test/socket.jl would seem like a natural place

@abhijithanilkumar
Copy link
Contributor Author

@tkelman Is this okay?

@tkelman
Copy link
Contributor

tkelman commented Dec 19, 2015

I think that looks right. repr shouldn't result in any stdout output, I think.

@tkelman
Copy link
Contributor

tkelman commented Dec 19, 2015

Once this is merged we should open a new issue for the other methods that don't work on UDPSocket for the same underlying reason that @rohitvarkey described above.

@abhijithanilkumar
Copy link
Contributor Author

@tkelman So is the work here done? Anything else to be covered?

@tkelman tkelman removed the needs tests Unit tests are required for this change label Dec 21, 2015
tkelman added a commit that referenced this pull request Dec 21, 2015
Fix show() error while calling UDPSocket() in REPL.
@tkelman tkelman merged commit f0e2ec1 into JuliaLang:master Dec 21, 2015
tkelman pushed a commit that referenced this pull request Jan 9, 2016
(cherry picked from commit 43c9583)
ref #14325
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.

4 participants