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

readbytes and readall, replaced by read and readstring per #14660 #162

Closed
wants to merge 3 commits into from

Conversation

samoconnor
Copy link
Contributor

Hi, @kmsquire, @nalimilan, re JuliaIO/GZip.jl#45 (comment): I've made an attempt. I haven't looked at Compat.jl before, so I'm not sure if I'm doing it right.

const readall = readstring
const readbytes = read
export readall
export readbytes
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it's at least reversed. Also I believe only readstring is a new name. Other ones shouldn't be overwirtten.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the tips @yuyichao!

@yuyichao
Copy link
Contributor

And please use a more descriptive commit and PR title.

@samoconnor samoconnor changed the title https://github.com/JuliaLang/julia/pull/14660 readbytes and readall, replaced by read and readstring Jan 21, 2016
@samoconnor samoconnor changed the title readbytes and readall, replaced by read and readstring readbytes and readall, replaced by read and readstring per #14608 Jan 21, 2016
@samoconnor samoconnor changed the title readbytes and readall, replaced by read and readstring per #14608 readbytes and readall, replaced by read and readstring per #14660 Jan 21, 2016
@jrevels
Copy link
Member

jrevels commented Jan 28, 2016

@samoconnor Could you rebase against the current master and add some tests? Otherwise, LGTM. This will fix a lot of annoying deprecation warnings for web-related stuff.

jrevels added a commit to JuliaCI/BaseBenchmarks.jl that referenced this pull request Jan 28, 2016
@samoconnor
Copy link
Contributor Author

@jrevels, done

@tkelman
Copy link
Contributor

tkelman commented Jan 30, 2016

ERROR: mktempdir has no method matching mktempdir(::Function) on 0.3 (that could be added as a new feature here in a separate PR though), and ERROR: LoadError: MethodError: Cannot convert an object of type Array{Any,1} to an object of type UTF8String on nightly.

@samoconnor
Copy link
Contributor Author

Hi @tkelman,
I don't have a nightly build to test this against, but it works with the 0.5 build I do have (Version 0.5.0-dev+2236 (2016-01-21 23:04 UTC)).
Is this possibly a type-inference regression in nightly?

This is the code that is failing:

        for text in [
            old_text,
            UTF8String(['A' + i % 52 for i in 1:(div(Base.SZ_UNBUFFERED_IO,2))]),
            UTF8String(['A' + i % 52 for i in 1:(    Base.SZ_UNBUFFERED_IO -1)]),
            UTF8String(['A' + i % 52 for i in 1:(    Base.SZ_UNBUFFERED_IO   )]),
            UTF8String(['A' + i % 52 for i in 1:(    Base.SZ_UNBUFFERED_IO +1)])
        ]

It looks to me that the compiler ought to be able to tell that those are Char arrays:
Char + Int % Int = Char

@tkelman
Copy link
Contributor

tkelman commented Feb 1, 2016

A type inference issue seems likely. Adding the Char[...] element type might work around the issue for now, but this could be a bug worth opening on base as well.

@samoconnor
Copy link
Contributor Author

@@ -560,3 +560,199 @@ Base.remote_do(() -> true, 1) # Doesn't return anything so cannot be `@test`ed b
@test "1234" == @compat withenv("_TESTVAR" => 1234) do
return ENV["_TESTVAR"]
end

# https://github.com/JuliaLang/julia/pull/14660
mktempdir() do dir
Copy link
Member

Choose a reason for hiding this comment

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

This relies on JuliaLang/julia#9017, which was introduced in Julia 0.4.

Probably we should have a Compat shim for that mktempdir feature on 0.3 as well.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see @tkelman commented on this above.

@malmaud
Copy link
Contributor

malmaud commented Feb 7, 2016

How's this PR doing? Can I help? Would love to see Requests.jl green again.

@tkelman
Copy link
Contributor

tkelman commented Feb 7, 2016

A separate addition of mktempdir should probably be the only thing this needs. We could do that in a separate PR, once that's good to merge we can just restart the CI builds here and they should pass.

@malmaud
Copy link
Contributor

malmaud commented Feb 7, 2016

OK, I'll do that today.

On Sun, Feb 7, 2016 at 12:32 PM Tony Kelman notifications@github.com
wrote:

A separate addition of mktempdir should probably be the only thing this
needs. We could do that in a separate PR, once that's good to merge we can
just restart the CI builds here and they should pass.


Reply to this email directly or view it on GitHub
#162 (comment).

@samoconnor
Copy link
Contributor Author

Hi @malmaud, that would me much appreciated.
I'm afraid I'm not at all familiar with Compat.jl, so this PR is just my best attempt.
I do all my work against v0.4.
I occasionally build a v0.5 environment if I'm trying to submit a bug fix, but I don't have any setup for testing against v0.3.
If you find anything else is broken in this PR please feel free to hijack my branch into your own fork and fix it up before merging with master.

@tkelman
Copy link
Contributor

tkelman commented Feb 8, 2016

You can download and extract a tarball binary to test against 0.3. It couldn't be any easier.

@malmaud
Copy link
Contributor

malmaud commented Feb 8, 2016

@samoconnor OK, I'll take you up on that

@tkelman
Copy link
Contributor

tkelman commented Feb 8, 2016

thanks @malmaud, do open a new pr so appveyor runs

@tkelman tkelman closed this Feb 8, 2016
@malmaud malmaud mentioned this pull request Feb 8, 2016
martinholters pushed a commit to martinholters/Compat.jl that referenced this pull request Jul 13, 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.

6 participants