Fix Compat on master by updating walkdir #307

andreasnoack · 2017-01-18T19:20:51Z

Since walkdir returns a Task in 0.5 and a Channel in 0.6 the implementation in Compat would need to be updated. For now, I've done this by not exporting the Compat version such that you'd have to use Compat.walkdir instead of just walkdir.

promote_eltype_op is now deprecated on master so I've also deprecated the Compat version.

Testing of vectorized round, ceil, floor, and trunc is disabled on 0.6 since the dot syntax should be used.

Some minor syntax deprecations.

TotalVerb · 2017-01-18T19:28:22Z

We shouldn't deprecate promote_eltype_op because that adds deprecation warnings to packages that use it on 0.5.

TotalVerb · 2017-01-18T19:29:45Z

I have an alternative approach here: https://github.com/TotalVerb/Compat.jl/commit/8f6d272edd06f845fbab66f9d138027fa3622186

This will deprecate promote_eltype_op only on 0.6. Do you think that approach would work?

andreasnoack · 2017-01-18T19:35:37Z

I'm not completely sure about what is the right solution here but Compat provides most recent Julia features on older Julia version. promote_eltype_op is no longer a Julia feature so I don't think that Compat should provide the feature.

TotalVerb · 2017-01-18T19:40:08Z

There may be packages that still support 0.4 and 0.5, and have yet to upgrade to 0.6. Compat should not break these packages or add deprecation warnings to them until Compat drops support for those versions. (Part of a point of a stable release, like 0.4/0.5, is to not have code suddenly give deprecation warnings. Other packages do deprecate things all the time, but Compat should be extra careful.)

That this feature is deprecated in Julia 0.6 means that we should deprecate it for Julia 0.6, but not 0.5 or earlier.

TotalVerb · 2017-01-18T19:44:02Z

src/Compat.jl

-if VERSION < v"0.5.0-dev+961"
-    export walkdir
-
+if VERSION < v"0.6.0-dev.2043"
    function walkdir(root; topdown=true, follow_symlinks=false, onerror=throw)


should document that walkdir should be used as Compat.walkdir on 0.5

why doesn't the base walkdir work on 0.5?

you can't take! from a Task, so the two walkdirs are not compatible

TotalVerb · 2017-01-18T19:45:15Z

src/Compat.jl

@@ -152,21 +152,21 @@ elseif VERSION < v"0.4.0-dev+6987"
    export pipeline
 end

-if VERSION < v"0.5.0-dev+961"


should still export walkdir on 0.4

isn't the semantic change breaking for anyone using this on 0.4?

Yes, any code using consume(walkdir(...)) will break. I don't know how to address this without keeping around two versions of walkdir.

export an old version on 0.4, have a non exported new version

TotalVerb · 2017-01-18T19:48:02Z

It would be breaking to unexport walkdir on 0.4. It's fine to restrict it to Compat.walkdir for 0.5.

andreasnoack · 2017-01-18T19:59:52Z

I think it is flawed to support the syntax of several versions of Julia at the same time and not only the latest version but I guess we are already doing that so I'll remove the part of this PR that touches promote_eltype_op and adjust the walkdir part.

tkelman · 2017-01-18T20:55:18Z

src/Compat.jl

-        dirs = Array(eltype(content), 0)
-        files = Array(eltype(content), 0)
+        dirs = Array{eltype(content)}(0)
+        files = Array{eltype(content)}(0)


This code is copied from Base; I don't think Compat should deviate from it.

it should be Vector in base then

The Array{T}(0) pattern is used very frequently in Base. Should that be changed in Base, then it can be changed here too.

it should, and has been in many places - it's easier on the compiler to call the constructor of a fully parameterized type instead of having to infer the dimensionality

I couldn't find a Base issue so opened JuliaLang/julia#20114.

tkelman · 2017-01-18T20:57:58Z

test/runtests.jl

-@test trunc(Int, [1 1]) == [1 1]
-@test trunc(Int, [1.1 1.1]) == [1 1]
-@test trunc(Int, fill(1.1, 2, 3, 4)) == fill(1, 2, 3, 4)
+if VERSION < v"0.6.0-"


be more specific, and anything called on scalars should run everywhere. or better, do @compat and dot syntax everywhere

Dot syntax doesn't work on 0.4 and 0.5 yet for these functions, see #297.

okay, then anything on scalars should run everywhere, anything vectorized on the specific julia versions where it's not deprecated

amitmurthy · 2017-01-19T06:28:31Z

test/runtests.jl

@@ -981,29 +983,29 @@ cd(dirwalk) do
    follow_symlink_vec = has_symlinks ? [true, false] : [false]
    has_symlinks && symlink(abspath("sub_dir2"), joinpath("sub_dir1", "link"))
    for follow_symlinks in follow_symlink_vec
-        task = walkdir(".", follow_symlinks=follow_symlinks)
-        root, dirs, files = consume(task)
+        task = Compat.walkdir(".", follow_symlinks=follow_symlinks)


Should also rename "task", "task_error", etc to "chnl", "chnl_error" everywhere.

andreasnoack · 2017-02-01T19:02:24Z

I've updated the PR such that it doesn't touch promote_eltype_op . Unfortunately, #315 happened meanwhile. I'm not sure yet what the right fix for that is.

andreasnoack · 2017-02-01T19:32:38Z

I've added a fix for #315 as well. It disables the same tests as in https://github.com/JuliaLang/julia/pull/18777/files#diff-3d2d2cb368aae0105589988195f6b11aL341 .

tkelman · 2017-02-01T19:42:08Z

Compat should not break these packages or add deprecation warnings to them until Compat drops support for those versions.

And multiple outstanding line comments.

andreasnoack · 2017-02-01T20:47:07Z

@amitmurthy I guess that Channels have changed quite a bit on master. On 0.5 they don't accept a function argument. Is at all feasible to support this in Compat?

amitmurthy · 2017-02-02T03:30:39Z

task-channel binding is not supported on 0.5

Compat's implementation of walkdir should

create an explicit task
close the channel before exiting - normal or on error.

I can push a commit to your branch later if you don't mind.

tkelman · 2017-02-02T03:53:09Z

README.md

@@ -81,7 +81,7 @@ Currently, the `@compat` macro supports the following syntaxes:

 * `foreach`, similar to `map` but when the return value is not needed ([#13744])

-* `walkdir`, returns an iterator that walks the directory tree of a directory ([#13707])
+* `walkdir`/`Compat.walkdir`, returns an iterator that walks the directory tree of a directory. For compatibility with Julia 0.6- `Compat.walkdir` should be used and `walkdir` should be used for compatibility with the Julia 0.5. ([#13707])


"for compatibility with Julia 0.5"

Keno

LGTM

No longer relevant. The approach to walkdir is different now

Keno · 2017-02-02T04:37:21Z

test/runtests.jl

-@test trunc(Int, fill(1.1, 2, 3, 4)) == fill(1, 2, 3, 4)
+
+if VERSION < v"0.6.0-dev.1825"
+    @test round(Int, [1, 1]) == [1, 1]


@compat round.(?

I see thanks. I don't care too strongly about these tests on 0.6, so I'm fine with this.

tkelman · 2017-02-02T04:38:27Z

src/Compat.jl

@@ -194,6 +194,9 @@ if VERSION < v"0.5.0-dev+961"
        Task(_it)
    end
 end
+if VERSION < v"0.6.0-dev.2043"
+    Base.take!(t::Task) = consume(t)


should be mentioned in readme

It is ~~not~~now mentioned in the README.md.

not = now ?

Make trunc, round, floor, ceil testing more precise

Keno · 2017-02-02T20:30:01Z

Tests failing on nightly now.

andreasnoack · 2017-02-02T20:35:22Z

Just realized. I think the deprecations caused centralizedabs2fun to be exported from Base so when the deprecations were deleted, centralizedabs2fun was no longer exported. I'm trying to come up with a fix.

depended on the deprecation definition in Base so it is kind of broken but I have left the definition to minimize the effects of this PR.

Keno · 2017-02-03T04:09:08Z

I think this is good to go. @andreasnoack do the honors?

Was added in #307.

* Remove `take!(::Task)` definition for Julia versions prior to 0.6 Was added in #307. * Remove `redirect_std*(f, stream)` definitions for Julia prior to v0.6 Were added in #275. * Remove at-__DIR__ macro definition for Julia versions prior to 0.6 Was added in #281. * Remove `broadcast` definition for equal-length tuples Was added in #324 and #328 * Remove definitions of `unsafe_get` and `isnull` fallsback Were added in #287. * Remove defintions of `xor` and `⊻` Were added in #289. * Definitions of `numerator` and `denominator` Were added in #290. * Remove defintion of `iszero` Was added in #305. * Remove definition of `>:` Was added in #336 * Remove definition of `take!(::Base.AbstractIOBuffer)` Was added in #290. * Remove definiton of `.&` and `.|` Were added in #306. * Remove definition of `Compat.isapprox` Was added in #309.

TotalVerb previously requested changes Jan 18, 2017

View reviewed changes

tkelman reviewed Jan 18, 2017

View reviewed changes

amitmurthy reviewed Jan 19, 2017

View reviewed changes

Only test vectorized round, ceil, floor, and trunc on 0.4 and 0.5.

40bc120

andreasnoack force-pushed the anj/06 branch from 992a7a2 to c3ea619 Compare February 1, 2017 18:58

andreasnoack force-pushed the anj/06 branch 2 times, most recently from b83b3b5 to 8053b6f Compare February 1, 2017 19:08

andreasnoack force-pushed the anj/06 branch from 8d3b4df to 5fbfa3d Compare February 1, 2017 19:59

andreasnoack force-pushed the anj/06 branch from 5fbfa3d to 8b4de1f Compare February 2, 2017 03:51

tkelman reviewed Feb 2, 2017

View reviewed changes

andreasnoack force-pushed the anj/06 branch 2 times, most recently from 16ffa63 to 4b5e947 Compare February 2, 2017 04:00

Define take!(t::Task) = consume(t)

d66eef2

andreasnoack force-pushed the anj/06 branch from 4b5e947 to 92e108c Compare February 2, 2017 04:03

Keno approved these changes Feb 2, 2017

View reviewed changes

Keno reviewed Feb 2, 2017

View reviewed changes

tkelman reviewed Feb 2, 2017

View reviewed changes

Update to linspace changes in #18777

4e8b7b3

Make trunc, round, floor, ceil testing more precise

andreasnoack force-pushed the anj/06 branch from 92e108c to 4e8b7b3 Compare February 2, 2017 15:46

andreasnoack and others added 2 commits February 2, 2017 16:01

Stop testing centralizedabs2fun on newest version of Julia. The behavior

4cd83fd

depended on the deprecation definition in Base so it is kind of broken but I have left the definition to minimize the effects of this PR.

Remove deprecated -F option

64f9b01

andreasnoack merged commit 6b6dfc9 into master Feb 3, 2017

andreasnoack deleted the anj/06 branch February 3, 2017 11:29

martinholters added a commit that referenced this pull request Aug 31, 2018

Remove take!(::Task) definition for Julia versions prior to 0.6

c4d150d

Was added in #307.

martinholters added a commit that referenced this pull request Aug 31, 2018

Remove take!(::Task) definition for Julia versions prior to 0.6

7de9ea2

Was added in #307.

Fix Compat on master by updating walkdir #307

Fix Compat on master by updating walkdir #307

Conversation

andreasnoack commented Jan 18, 2017

TotalVerb commented Jan 18, 2017

TotalVerb commented Jan 18, 2017

andreasnoack commented Jan 18, 2017

TotalVerb commented Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TotalVerb Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TotalVerb commented Jan 18, 2017

andreasnoack commented Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreasnoack commented Feb 1, 2017

andreasnoack commented Feb 1, 2017

tkelman commented Feb 1, 2017

andreasnoack commented Feb 1, 2017

amitmurthy commented Feb 2, 2017

Choose a reason for hiding this comment

Keno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreasnoack Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keno commented Feb 2, 2017

andreasnoack commented Feb 2, 2017

Keno commented Feb 3, 2017

TotalVerb commented Jan 18, 2017 •

edited

Loading

TotalVerb Jan 18, 2017 •

edited

Loading

andreasnoack commented Jan 18, 2017 •

edited

Loading

andreasnoack Feb 2, 2017 •

edited

Loading