Updates to be compatible with 0.7 #52

morris25 · 2018-04-20T20:03:56Z

This PR deprecates any functions not supported by Base.NamedTuple in v0.7. It deprecates using @NT to declare a type so that:
@NT(a, b) -> Deprecated in favor of NamedTuple{(:a, :b)}
@NT(a::Int64, b::Float64) -> Deprecated in favor of NamedTuple{(:a, :b), Tuple{Int64, Float64}}
@NT(a, b)(1, "hello") -> Deprecated in favor of NamedTuple{(:a, :b)}(1, "hello")
@NT(a::Int64)(2.0) -> Deprecated in favor of NamedTuple{(:a,), Tuple{Int64}}(2.0)

@NT(a = 1, b = "hello") -> remains the same
@NT(::Int64, ::Float64) -> Deprecated in favor of new macro@nt(::Int64, ::Float64)

It also adds Appveyor.
Closes #51

omus · 2018-04-20T20:15:24Z

src/NamedTuples.jl

-        fields[i]  = sym != nothing?sym:Symbol( "_$(i)_")
-        typs[i] = typ
+        fields[i]  = sym != nothing ? sym : Symbol( "_$(i)_")
+        typs[i] = typ != nothing ? typ : Any


Use === or !== when comparing against `nothing.

omus · 2018-04-20T20:16:14Z

src/NamedTuples.jl

+        ty = create_namedtuple_type( fields )
+    else
+        ty = Expr(:curly, :NamedTuple, (fields...))
+    end


Should this just call make_tuple?

omus · 2018-04-20T20:32:07Z

The deprecations on 0.7 should also be addressed: https://travis-ci.org/JuliaData/NamedTuples.jl/jobs/369297897

omus · 2018-04-20T20:33:44Z

src/NamedTuples.jl

+export @NT
+
+if VERSION < v"0.7.0-DEV.2738"
+    export NamedTuple


It is fine to export NamedTuple on Julia 0.7

omus · 2018-04-20T20:36:49Z

src/NamedTuples.jl

    end
 end

+function create_struct_expr(name::Symbol, fields::Vector{Symbol}, mod::Module = NamedTuples)


mod isn't used

omus · 2018-04-20T20:39:33Z

src/NamedTuples.jl

+            return Expr( :curly, ty, typs... )
+        else
+           return Expr(:curly, ty, Expr(:curly, :Tuple, typs...))
+        end


Whitespace issues. Could we switch to using Tuple on 0.6?

omus · 2018-04-20T20:48:51Z

src/NamedTuples.jl

-    return q
-end
+    # Deep hash
+    @generated function Base.hash(nt::NamedTuple, hs::UInt64)


This looks to be the 32-bit issue. Should be hs::UInt and not hs::UInt64

omus · 2018-04-23T20:00:20Z

src/NamedTuples.jl

+"""
+function NamedTuple{names}(args...) where {names}
+    NamedTuple{names}(args)
+end


I think you want:

function NamedTuple{names}(args...) where {names} NamedTuple{names, typeof(args)}(args) end

These are just helper functions to allow NamedTuple(args) rather than NamedTuple((args)) fixing the types happens below. I wanted to let Base.NamedTuple deal with as much as possible as far as declarations go.

omus · 2018-04-23T20:01:42Z

src/NamedTuples.jl

+
+function NamedTuple{names,T}(args...) where {names, T <: Tuple}
+    NamedTuple{names, T}(args)
+end


function NamedTuple{names,T}(args...) where {names, T <: Tuple} NamedTuple{names, T}(args::T) end

This would break the conversion of @NT(a::Int64)(2.0) to NamedTuple{(:a,), Tuple{Int64}}(2.0)

omus · 2018-04-23T20:20:30Z

src/NamedTuples.jl

+    # TODO: to make modules containing named tuples precompile-able, change `= NamedTuples` to `= current_module()`
+    function create_namedtuple_type(fields::Vector{Symbol}, mod::Module = NamedTuples)
+        escaped_fieldnames = [":$i" for i in fields]
+        name =Symbol(string("NamedTuple{(", join(escaped_fieldnames,", "), ",)}"))


I think the original was less confusing when the type was printed. That said we should probably do:

function Base.show(io::IO, ::Type{<:NamedTuple{K, T}}) where {K, T<:Tuple} print(io, "NamedTuple{$K, $T}") end

to display the type nicely on 0.6.

Changes `NamedTuple` to `NamedTuple{K, V<:Tuple}` and moves expr construction to separate function

- Removes deprecated doc formatting - Disables functions implemented in Base - Uses Base `NamedTuple` instead of local struct - Disables tests for broadcast (intentionally disabled in Base) - Allows `NamedTuple{K,T}()` construction format

Fixes 0.7 deprecations Adds macro to replace auto-naming functionality

omus · 2018-04-24T16:56:11Z

src/NamedTuples.jl

        end
+        print(io, ")")
    end


The printing for a NamedTuple with a single field should have a trailing comma: (a = 1,) instead of (a = 1)

In Julia 0.7:

julia> (a=1) 1 julia> (a=1,) (a = 1,)

omus · 2018-04-24T17:06:40Z

src/NamedTuples.jl

+macro NT( expr... )
+    args = collect( expr )
+    if isa(args, Vector{Symbol})
+        Base.depwarn("\"@NT( a, b )\" syntax for NamedTuple Type declaration is deprecated, use \"NamedTuple{(:a, :b)}\" instead.", Symbol("@NT"))


Should be dynamic based upon the inputs:

Base.depwarn("\"@NT($(join(expr, ", ")))\" syntax for NamedTuple Type declaration is deprecated, use \"NamedTuple{$(repr(expr))}\" instead.", Symbol("@NT"))

would result in:

julia> @NT(c, d) WARNING: "@NT(c, d)" syntax for NamedTuple Type declaration is deprecated, use "NamedTuple{(:c, :d)}" instead.

"Type" should be lowercase

davidanthoff · 2018-04-24T18:23:40Z

Just a warning/heads up: I will upper bound all the packages from the Queryverse to not use this version on julia 0.6. That is a lot of packages, and some of them quite low-level, so in essence that would probably mean that a lot of folks would never get this update on 0.6.

My migration plan for Queryverse right now is to just do a breaking release once 0.7 is out that only works on 0.7, i.e. drop 0.6 support at that time. I do not plan to have a version that supports both 0.6 and 0.7 at the same time.

omus · 2018-04-24T19:32:18Z

src/NamedTuples.jl


    # Either call the constructor with the supplied values or return the type
    if( !construct )
        if len == 0
+            Base.depwarn("\"@NT( a, b )\" syntax for NamedTuple Type declaration is deprecated, use \"NamedTuple{(:a, :b)}\" instead.", Symbol("@NT"))


Hmm, we probably shouldn't make an empty named tuple. Probably this should be an error which raises an error before we construct the type.

omus · 2018-04-24T19:42:59Z

My migration plan for Queryverse right now is to just do a breaking release once 0.7 is out that only works on 0.7, i.e. drop 0.6 support at that time. I do not plan to have a version that supports both 0.6 and 0.7 at the same time.

Completely reasonable plan. We still should try to support an easy transition for package that do want to support 0.6 and 0.7.

davidanthoff · 2018-04-24T19:46:10Z

I guess the question is whether that can be done without deprecations? The moment we introduce deprecations here, I'll have to add the upper bounds in my packages, and that will then hold things back for a lot of folks...

omus · 2018-04-24T19:51:17Z

I'm not sure what the issue with deprecations is. The deprecations which are being proposed here are to use a syntax which is much closer to what is being used in Julia 0.7 named tuples. If this gets merged there will be deprecations introduced for those using the NamedTuples package and the @NT macro for creating types. You would just need to update the deprecations in your own package like any other deprecation.

Overall this should help make your package work much closer to how it will work when you drop Julia 0.6 support.

omus · 2018-04-24T20:00:45Z

src/NamedTuples.jl

            return ty
        end
-        return Expr( :curly, ty, typs... )
+        syms = [repr(i) for i in fields]
+        Base.depwarn("\"@NT($(join(exprs, ", ")))\" syntax for NamedTuple type declaration is deprecated, use \"NamedTuple{($(join(syms, ", "))), Tuple{$(join(typs, ", "))}}\" instead.", Symbol("@NT"))


($(join(syms, ", ")) needs a trailing comma when there is only one element. This works join(syms, ", ") * (length(syms) == 1 ? "," : "") but is rather ugly

omus · 2018-04-24T20:06:55Z

REQUIRE

@@ -1 +1,2 @@
 julia 0.6
+Compat


Need to set a minimum version

Is Compat needed here or only for tests?

omus · 2018-04-24T20:13:49Z

README.md

+NamedTuple{(:a, :b)}                         -> Defines a tuple with a and b as members
+NamedTuple{(:a, :b), Tuple{Int64, Float64}}  -> Defines a tuple with the specific arg types as members
+
+@NT( a = 1, b = "hello").                    -> Defines and constructs a tuple with the specifed members and values


omus · 2018-04-24T20:24:27Z

One potential gotcha with this PR we may want to address in the README is that on Julia 0.7:

julia> t = (a=1,)
(a = 1,)

julia> typeof(t) == NamedTuple{(:a,), Tuple{Int64}}
true

However with NamedTuples on Julia 0.6:

julia> t = NamedTuple{(:a,), Tuple{Int64}}(1)
(a = 1,)

julia> typeof(t) == NamedTuple{(:a,), Tuple{Int64}}
false

julia> typeof(t) <: NamedTuple{(:a,), Tuple{Int64}}
true

julia> typeof(t)
NamedTuple{(:a,), Tuple{Int64}}

The new show method definitely adds to the confusion.

omus · 2018-04-24T20:30:01Z

src/NamedTuples.jl

-        $(gen_namedtuple_ctor_body(n, args))
+    # Pretty print type information
+    function Base.show(io::IO, ::Type{<:NamedTuple{K, T}}) where {K, T<:Tuple}
+        print(io, "NamedTuple{$K, $T}")


I change my mind with this. We should remove it to avoid confusion. See: #52 (comment)

omus · 2018-04-24T20:32:50Z

src/NamedTuples.jl

-
-    return q 
+if VERSION < v"0.7.0-DEV.2738"
+    abstract type NamedTuple{K, V<:Tuple} end


Lets change this to:

abstract type NamedTuple{names, T<:Tuple} end

to match the type parameter variables used in Julia 0.7: https://github.com/JuliaLang/julia/blob/9f5351c36abb1cc7c7e1df9b1b73df696401d13a/base/namedtuple.jl#L58

omus · 2018-04-24T20:39:26Z

test/runtests.jl

+nt = NamedTuple{(:a, :b), Tuple{Empty, Int}}
+@test nt.parameters[2] == Tuple{Empty, Int}


Test is using the abstract type rather than the constructed struct. See: #52 (comment)

davidanthoff · 2018-04-24T21:14:31Z

I'm not sure what the issue with deprecations is.

I just creates extra work: instead of just having one update cycle once julia 0.7 comes out, I would have to update packages twice, now to fix the deprecations, and then again when julia 0.7 comes around. At least for me the solution would be to just not fix the deprecations on 0.6 and instead add upper bounds in REQUIRE. From my point of view that works, but because some of the packages I would upper bound are so deep in the stack, it would mean that a lot of systems would never see this new release here, which might kind of defeat the purpose (not sure).

omus · 2018-04-24T20:50:38Z

test/runtests.jl


-@test typeof( @NT( a::Int64, b::Float64 )(1, 3.0) ) == typeof( @NT( a = 1, b = 2.0 ))
+@test typeof( NamedTuple{(:a,:b), Tuple{Int,Float64}}(1, 3.0) ) == typeof( @NT( a = 1, b = 2.0 ))


Would be useful tests to add:

@test typeof(@NT(a = 1, b = 2.0)) <: NamedTuple{(:a, :b), Tuple{Int,Float64}} if VERSION < v"0.7.0-DEV.2738" @test typeof(@NT(a = 1, b = 2.0)) != NamedTuple{(:a, :b), Tuple{Int,Float64}} else @test typeof(@NT(a = 1, b = 2.0)) == NamedTuple{(:a, :b), Tuple{Int,Float64}} end

omus · 2018-04-24T20:52:33Z

test/runtests.jl

-@test @NT( a ) ==  @NT( a )
-@test @NT( a ) !=  @NT( b )
+@test NamedTuple{(:a,)} ==  NamedTuple{(:a,)}
+@test NamedTuple{(:a,)} !=  NamedTuple{(:b,)}


Test should be comparing the constructed struct types not the abstract type

omus · 2018-04-24T21:28:46Z

@davidanthoff it seems odd to me that you're willing to put in the work to change the REQUIRE file to add an upper bound to a requirement but not revise the code to clean up deprecations.

I'll definitely say wait and see until this PR has stabilized before making any choices as to upper bounding or updating. If you do think it's too much work to support the next release I can (or one of my team members) do the update on your behalf.

omus · 2018-04-24T21:38:13Z

After looking over this code it may be best if we divide some of the functionality like this:

NamedTuple: Concrete type in Julia 0.7 and an abstract type in Julia 0.6. Useful in function type declarations where you want to use the same syntax for both versions of Julia.
@NT: Macro used to make concrete NamedTuple types. In Julia 0.7 this is just returning NamedTuple while in Julia 0.6 this may need to define new concrete types. The use of this macro for creating named tuple instances will be deprecated.
@nt: Macro used to only create instances of named tuples. Mirrors the short named tuple syntax (e.g. (a=1,)) supported on Julia 0.7

If we want to go 100% deprecation free we could leave @NT as is. The disadvantage of doing that is that it is less clear whether the macro will be returning a type or an instance.

davidanthoff · 2018-04-25T17:51:50Z

it seems odd to me that you're willing to put in the work to change the REQUIRE file to add an upper bound to a requirement but not revise the code to clean up deprecations.

Well, I think the etiquette in METADATA is that if you want to tag a new version that breaks/negatively affects other packages, you have to put the upper bounds into METADATA, so that should be no work for me :)

This is really just about my time budget: I just don't have the time right now to do two sweeps of deprecation updates, once for this and then again for 0.7. Introducing breaking changes that low in the stack is just hugely expensive for me. My guess is that there are about 20 packages that might need updating/testing, everything needs to be synchronized, tested in sync etc. Some of those packages I control, others I don't. A lot of that probably can't be outsourced to someone else, all the tagging/testing/coordinating would probably end up on my plate at the end of the day.

I guess my preference in general is that packages that have many dependencies and are low in the stack are super, super conservative in terms of deprecations/breaking changes. If they do make breaking changes, it just causes this enormous ripple effect of extra work for other folks. Sometimes that can't be avoided, but it would be great if it could be rare and we would try to avoid when at all possible.

I guess another option might be to just fork NamedTuples.jl: create a NamedTuples2.jl that works on julia 0.6 and 0.7, and folks that want to support both platforms could use that. The benefit of that would be that other packages could just stay on NamedTuples.jl for julia 0.6 and wouldn't require any extra work right now.

omus · 2018-04-25T20:18:00Z

In an effort to keep the changes to support Julia 0.7 to a minimum the critical parts of this PR have been made into #55.

omus reviewed Apr 20, 2018

View reviewed changes

src/NamedTuples.jl Outdated

export @NT

if VERSION < v"0.7.0-DEV.2738"

export NamedTuple

Copy link

Member

omus Apr 20, 2018

Choose a reason for hiding this comment

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

It is fine to export NamedTuple on Julia 0.7

omus reviewed Apr 20, 2018

View reviewed changes

omus reviewed Apr 23, 2018

View reviewed changes

morris25 changed the title ~~WIP: Updates to be compatible with 0.7~~ Updates to be compatible with 0.7 Apr 23, 2018

omus mentioned this pull request Apr 24, 2018

Roadmap for this NamedTuples package after NamedTuples being merged in Julia master branch #45

Open

morris25 added 4 commits April 24, 2018 10:20

Deprecates functions not supported in 0.7

95d8ee8

Changes `NamedTuple` to `NamedTuple{K, V<:Tuple}` and moves expr construction to separate function

Adds Appveyor script

8871b1b

Updates Pkg to work on v0.7

f2a29ea

- Removes deprecated doc formatting - Disables functions implemented in Base - Uses Base `NamedTuple` instead of local struct - Disables tests for broadcast (intentionally disabled in Base) - Allows `NamedTuple{K,T}()` construction format

Deprecates NT macro as type declaration

7474f7d

Fixes 0.7 deprecations Adds macro to replace auto-naming functionality

omus reviewed Apr 24, 2018

View reviewed changes

morris25 added 2 commits April 24, 2018 13:46

Fixes show and deprecation formatting

0e81a95

Deprecates make_tuple(fields::Vector{Symbol})

a48b47b

omus reviewed Apr 24, 2018

View reviewed changes

omus mentioned this pull request Apr 24, 2018

Create a NamedTuple from a tuple of symbols and a tuple of values #53

Open

omus mentioned this pull request Apr 25, 2018

Minimal changes to work on Julia 0.7 #55

Merged

omus mentioned this pull request Apr 25, 2018

Support for testing on AppVeyor #56

Merged

morris25 closed this Nov 16, 2018

		nt = NamedTuple{(:a, :b), Tuple{Empty, Int}}
		@test nt.parameters[2] == Tuple{Empty, Int}


		@test typeof( @NT( a::Int64, b::Float64 )(1, 3.0) ) == typeof( @NT( a = 1, b = 2.0 ))
		@test typeof( NamedTuple{(:a,:b), Tuple{Int,Float64}}(1, 3.0) ) == typeof( @NT( a = 1, b = 2.0 ))

Updates to be compatible with 0.7 #52

Updates to be compatible with 0.7 #52

Conversation

morris25 commented Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus commented Apr 20, 2018

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

morris25 Apr 23, 2018 • 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

davidanthoff commented Apr 24, 2018

Choose a reason for hiding this comment

omus commented Apr 24, 2018

davidanthoff commented Apr 24, 2018

omus commented Apr 24, 2018

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

omus commented Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidanthoff commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus commented Apr 24, 2018

omus commented Apr 24, 2018 • edited Loading

davidanthoff commented Apr 25, 2018

omus commented Apr 25, 2018

morris25 commented Apr 20, 2018 •

edited

Loading

morris25 Apr 23, 2018 •

edited

Loading

omus commented Apr 24, 2018 •

edited

Loading

omus commented Apr 24, 2018 •

edited

Loading