JuliaData / NamedTuples.jl

[DEPRECATED] NamedTuples.jl
Other
30 stars 17 forks source link

Minimal changes to work on Julia 0.7 #55

Closed omus closed 6 years ago

omus commented 6 years ago

Uses the minimal required changes from #52 to support named tuples on Julia 0.6 and use built-in named tuples on Julia 0.7.

To properly support the build-in named tuples on Julia 0.7 we needed to deprecate the constructor (::Type{NT})(args...) where {NT <: NamedTuple} which is ambiguous with the NamedTuple{...}(t::Tuple) when a single tuple is passed in. Unfortunately making this change is breaking as the behaviour of @NT(a)((1,)) will be (a=(1,),) with the old constructor and (a=1,) with the new one.

I'll also note I did not indent code with VERSION blocks as it made the diff hard to read. I can either add the indentation in another PR or as an additional commit to this PR.

omus commented 6 years ago

AppVeyor failures are just on 32-bit which will be addressed by: https://github.com/JuliaData/NamedTuples.jl/pull/56

omus commented 6 years ago

Note that these changes do not fix Julia 0.7 deprecations. The deprecations for this package will be taken care of in another PR.

omus commented 6 years ago

I've been looking at the packages which depend NamedTuples to see what kind of repercussions this deprecation will have. After looking at some code specifically IndexedTables we definitely need to have a constructor for NamedTuples where an iterable is provided. This feature was missing in Julia 0.7 so I added it here: https://github.com/JuliaLang/julia/pull/26914

omus commented 6 years ago

Unfortunately we cannot have a non-breaking change here as the behaviour of @NT(a)((1,)) would be (a=(1,),) without this PR and (a=1,) with this PR. The safest course of action will be to upper bound packages dependent on NamedTuples and include these changes as a new major release.

In order to make the transition process slightly smoother I've introduced a "breaking deprecation" where unambiguous usage of the old constructor will produce an exception. Additionally, @morris25 and I will be making PRs to packages depending on NamedTuples to support this new version.

davidanthoff commented 6 years ago

Couldn't we do the truly minimal thing here, i.e. fix deprecation warnings and errors on 0.7, but nothing else? Yes, it wouldn't be based on the native named tuples in 0.7, but we wouldn't force folks to go through a major round of changes on 0.6, which seems a major benefit (especially with 0.7 not even having released an alpha).

If folks then want to use 0.7 named tuples, they can, on their own schedule. Or maybe it could even be added to Compat.jl (where this kind of backporting of future features really belongs).

omus commented 6 years ago

There is the potential to make the existing code work as is without switching to the built in named tuples on Julia 0.7. However, there are still some challenges with that approach. One major issues is that package using NamedTuples will no longer export the NamedTuple abstract type defined:

julia> using NamedTuples

julia> NamedTuple
WARNING: both NamedTuples and Core export "NamedTuple"; uses of it in module Main must be qualified
ERROR: UndefVarError: NamedTuple not defined

I think the best approach since supporting Julia 0.7 will require some changes for packages that use NamedTuples.jl is the one proposed here.

omus commented 6 years ago

Or maybe it could even be added to Compat.jl (where this kind of backporting of future features really belongs).

Generally, I agree with you. However the Missings.jl package gives us a precedent for having packages back port future features.

davidanthoff commented 6 years ago

However, there are still some challenges with that approach. One major issues is that package using NamedTuples will no longer export the NamedTuple abstract type defined

One can just use import NamedTuples and then use NamedTuples.NamedTuple to refer to things if one wants stuff to work on 0.7, so no code would be broken on 0.6.

I think the best approach since supporting Julia 0.7 will require some changes for packages that use NamedTuples.jl is the one proposed here.

I have no problem that packages will have to do work to support 0.7. But breaking things on 0.6 at this point is a major headache and I would prefer that we don't do that.

Generally, I agree with you. However the Missings.jl package gives us a precedent for having packages back port future features.

That seems a very different case, as nothing on 0.6 was broken by that approach.

In general, can we have a process for decision like this here that involves the major users and comes to some sort of consensus about a strategy before we start to migrate lots of code to a new proposal?

omus commented 6 years ago

One can just use import NamedTuples and then use NamedTuples.NamedTuple to refer to things if one wants stuff to work on 0.7, so no code would be broken on 0.6

I was aware of this. My point is that it the change may still require changes to dependent packages.

I have no problem that packages will have to do work to support 0.7. But breaking things on 0.6 at this point is a major headache and I would prefer that we don't do that.

I understand this position. I'm not planning on merging this right away without a discussion but I think the full discussion will be had when we have WIP PRs which guarantee an upgrade path for all of the dependent packages. Additionally, if we decide we want to do this I would take care of applying the upper-bounded in METADATA such that this is a transparent non-breaking transition. The only thing which would be required from you would be to review the WIP PRs and make a new release. I'm hoping this is acceptable?

In general, can we have a process for decision like this here that involves the major users and comes to some sort of consensus about a strategy before we start to migrate lots of code to a new proposal?

I was going to wait until we had all of the WIP PRs ready but we can start the dialog earlier. I'll make a posting in #data in the JuliaLang Slack to get things started. Did you have any other suggestions here?

davidanthoff commented 6 years ago

Can we have the discussion here in an issue? Slack seems fine for short questions etc., but for real design discussions it really is not ideal: messages disappear after a while, lots of topics get mixed up, not everyone is on slack etc.

nalimilan commented 6 years ago

I agree with @davidanthoff that it would be too bad to break packages which use NamedTuples.jl on 0.6. Upper bounds in METADATA are better avoided when possible (see the less than satisfactory dependency incompatibilities with DataFrames 0.11).

Can't the @NT constructor be adjusted so that it continues to behaves as it currently does, even if that requires ugly special-case checks on 0.7?

omus commented 6 years ago

Can't the @NT constructor be adjusted so that it continues to behaves as it currently does, even if that requires ugly special-case checks on 0.7?

The issue isn't with @NT but rather cases where you're constructing an instance of a named tuple from a named tuple type. For example:

julia> T = @NT(a, b)
NamedTuples._NT_a_b

julia> T(1, 2)  # Uses Vararg constructor
(a = 1, b = 2)

The main issue is that NamedTuples.jl uses a vararg constructor while Julia 0.7 uses a tuple constructor:

NT(args...) = ...  # NamedTuples.jl
NamedTuple{...}(args::Tuple) = ...  # Julia 0.7

The breaking change occurs when we have both constructors at once. It becomes impossible to know what behaviour we want when passed in a single tuple:

Current NamedTuples.jl

julia> @NT(a)(tuple(2))
(a = (2,))

Julia 0.7

julia> NamedTuple{(:a,)}(tuple(2))
(a = 2,)

Making matters worse even if type information is provided to the @NT constructor there are still cases where it ambiguous what behaviour should occur.

Current NamedTuples.jl

julia> @NT(a::Tuple)(tuple(tuple(1)))
(a = ((1,),))

PR which includes tuple constructor:

julia> @NT(a::Tuple)(tuple(tuple(1)))
(a = (1,))
omus commented 6 years ago

This PR brings the NamedTuples.jl behaviour inline with the current implementation already in Julia 0.7 which should make it easier to switch to Julia 0.7 when developers are ready to drop Julia 0.6.

nalimilan commented 6 years ago

What I suggest is to retain the current behavior of @NT where you say it's ambiguous. There's no reason why @NT needs to follow exactly the behavior of NamedTuple, is there?

omus commented 6 years ago

Given that we want all packages to switch to Julia 0.7 once it is out I'm not sure why there is pushback against switching to use the newer Julia 0.7 syntax. Since we are preparing PRs for all repos which depend on NamedTuples we will have a clear idea of what packages will required to be upper bounded and which packages will not require it. Furthermore, we'll have PRs ready to go the instant these changes are merged and tagged so the amount of time a possible package conflict could exist will be minimized.

nalimilan commented 6 years ago

Fair enough if you're ready to make PRs for all dependent packages.

omus commented 6 years ago

Upper bounding NamedTuples.jl to only work on Julia 0.6 or below is causing some headaches. One possible solution is to merge these changes and upper bound all existing uses of NamedTuples.jl. The other solution is to just have a release of NamedTuples.jl for Julia 0.7 which just exports the functionality in Base.

codecov-io commented 6 years ago

Codecov Report

Merging #55 into master will increase coverage by 2.62%. The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #55      +/-   ##
=========================================
+ Coverage   73.18%   75.8%   +2.62%     
=========================================
  Files           1       2       +1     
  Lines         179     186       +7     
=========================================
+ Hits          131     141      +10     
+ Misses         48      45       -3
Impacted Files Coverage Δ
src/deprecated.jl 0% <0%> (ø)
src/NamedTuples.jl 76.21% <69.23%> (+3.03%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6353fcc...8ee2198. Read the comment docs.

omus commented 6 years ago

@davidanthoff any issues with the plan? I'm hoping to get this in ASAP

davidanthoff commented 6 years ago

Having a version of NamedTuples.jl that is 0.7 and up only and just exports the stuff from base might be the easiest option? That would require no upper bounds or anything like that on packages in METADATA, right?

The other option, i.e. releasing a breaking change on julia 0.6, could work if someone adds all the necessary bounds in METADATA, but I don't really see a great benefit to that. It seems more work, more risk that something can go wrong with these upper bounds and I don't really see a scenario that this would enable, as far as I can see.

At the end of the day I'm indifferent, as long as the strategy doesn't break existing code on julia 0.6 :)

omus commented 6 years ago

Having a version of NamedTuples.jl that is 0.7 and up only and just exports the stuff from base might be the easiest option? That would require no upper bounds or anything like that on packages in METADATA, right?

You are correct this approach would not require any upper bounds. I'll update this PR to set the Julia lower bound and I'll proceed with this. :)