Remove all RDA functionality from the package #1031

ararslan · 2016-08-10T20:55:01Z

~~NOTE: DO NOT MERGE UNTIL RDATA.JL IS REGISTERED!~~ RData has been registered.

Farewell RDA, you served us well, but you'll be happier in your new home.

Fixes #955.

nalimilan · 2016-08-16T14:10:08Z

Would there be a way to reexport RData.jl functions and print a deprecation warning when they are called via DataFrames.jl, but not after the user has called using RData? I'm afraid people will have a hard time discovering where the feature have gone.

ararslan · 2016-08-16T16:50:49Z

I figured we'd do it in a similar manner to how prime functionality was removed from Base. That's what I did here (not really obvious in the large diff).

Edit: Sorry, wrong link: https://github.com/JuliaStats/DataFrames.jl/blob/702e3bd0556b30416d503ffae8bbd8c62048a7af/src/deprecated.jl#L20

ararslan · 2016-08-17T16:53:33Z

If we continue to export read_rda here, we won't be able to have it exported in RData until after the deprecation period here. Maybe that's okay though, dunno.

nalimilan · 2016-08-18T15:25:19Z

We could also @reexport that function, since the deprecation warning printed by RData.jl will give useful instructions.

ararslan · 2016-08-18T17:07:59Z

That's a good idea.

nalimilan · 2016-08-22T13:43:28Z

src/deprecated.jl

@@ -1,18 +1,23 @@
-using Base.depwarn
+import Base: @deprecate, depwarn


import is not needed here as we don't extend these methods. Also, better have this in a separate commit.

nalimilan · 2016-08-22T13:45:37Z

The failures on all platforms are worrying...

ararslan · 2016-08-22T16:50:12Z

Indeed, though it appears to be caused by JuliaData/RData.jl#8. Once that's fixed, things should (hopefully) work as expected.

alyst · 2016-08-24T17:13:44Z

src/deprecated.jl

+
+function read_rda(args...)
+    depwarn("read_rda is deprecated. Use the load function from the RData package.")
+    RData.load(args...)


Here we should call FileIO.load(), because the file format-specific load() functions are not meant to be called directly from the user code (and they have special signatures), rather by FileIO.load() that delegates loading to specific package based on the current registry of formats. So the more accurate deprecation message would be something like:

`read_rda()` is deprecated. R data format support is moved to `RData` package. Use `load()` function.

It is also possible to remove circular dependency and replace RData with FileIO in REQUIRE. If read_rda() is called before using RData (i.e. ".rda" not registered), FileIO.load() will give an error about unsupported data type (or read_rda() could check whether ".rda" format is registered in FileIO and suggest the user to run using RData).

+1, but the message should be "use FileIO.load()".

As regards registering, you should make a PR against FileIO to associate RData with .rda files: https://github.com/JuliaIO/FileIO.jl/blob/master/src/registry.jl See also https://github.com/JuliaIO/FileIO.jl#adding-new-formats

@nalimilan add_format() is already called during RData initialization, but it's a good idea to register it in FileIO directly. In that regard, what extensions should we register? ".rda" and ".rdata"?

dmbates · 2016-08-24T20:39:29Z

@alyst It is more common to use the .RData extension than .rdata.

nalimilan · 2016-08-24T21:46:23Z

Better register .rda, .RData and .rdata. (The Freedesktop MIME database has the first and the last one, which also matches the second one as a fallback.)

alyst · 2016-08-26T15:11:17Z

FileIO v0.1.2 knows about .rda&co out of the box (JuliaIO/FileIO.jl#75), so it should be added to the DataFrame requirements instead of RData and read_rda() should call FileIO.load().
RData v0.0.4 supports the updated FileIO (JuliaData/RData.jl#15).

ararslan · 2016-08-30T16:54:29Z

The nightly OS X failure seems to be Travis' fault but I restarted that build just in case. That aside, is this good to go?

ararslan · 2016-08-30T17:49:28Z

Everything is nice and green now; the failure was indeed unrelated. ~~If no one objects, I'll go ahead and merge this a little later.~~ ~~Just realized there are a couple of comments I missed. Will get to those later today.~~ Sorry, confused myself. I did everything I had intended to do.

nalimilan · 2016-08-31T17:34:47Z

Good to see tests green again!

ararslan · 2016-09-05T17:44:55Z

It looks like the deprecation may not have been done properly, or else RDatasets wouldn't be broken as in #1054. (Also ref the fix in JuliaStats/RDatasets.jl#43) @nalimilan, any idea?

andreasnoack · 2016-09-05T17:49:58Z

I just hit this and I open a PR soon but adding a test was a little harder than expected.

tlnagy · 2016-09-05T18:52:14Z

I don't know where would be the best place to bring this up, but are there any plans to do more rigorous cross-package testing?

My beef with this split everything up into small contained packages is that the unit tests pass in each one, but changes get merged all the time that end up breaking downstream packages. It really doesn't really incite too much confidence in the package ecosystem as a whole if the rug can be pulled out from under your code at any point. When more code was included in the small package then unit tests encapsulated more of the whole and were a more reliable indicator of breakage.

ararslan · 2016-09-05T19:22:25Z

I'd love to see PkgEvaluator become a CI service, wherein a Travis-like check on a PR will tell you what packages will be broken by a change, but I don't think the resources currently exist to make that happen.

Pipe dreams aside, I agree that things like this can be frustrating for all involved. But I also think that smaller, more modularized packages help both with pinpointing the source of breakages and making packages less monolithic and easier to maintain. As long as packages have unit tests that exercise the code in their respective packages how it's intended to be used, I don't think it's too much of a problem for an issue with package X to be found by package Y; the developers of package X can't come up with an exhaustive list of every use case for their package, so it just isn't feasible to test everything at once. As long as everyone remains in communication and submits issues and PRs, I think what we have now is sustainable.

Others may feel differently, but that's my $0.02.

nalimilan · 2016-09-05T19:24:26Z

@ararslan The error from http://pkg.julialang.org/logs/Gadfly_0.5.log indicates that depwarn is called incorrectly: it needs a second argument giving the name of the function as a symbol.

andreasnoack · 2016-09-05T19:25:03Z

@nalimilan See #1056

tlnagy · 2016-09-05T19:42:00Z

As long as packages have unit tests that exercise the code in their respective packages how it's intended to be used, I don't think it's too much of a problem for an issue with package X to be found by package Y; the developers of package X can't come up with an exhaustive list of every use case for their package, so it just isn't feasible to test everything at once.

I think this is okay as long as release versions of packages don't break, if possible. That's what was a bit frustrating about this change was that it broke the release version of RDatasets before a patch was available.

ararslan · 2016-09-05T19:50:40Z

Yeah, in this case the breakage was totally my fault, the result of a stupid mistake. I sincerely apologize for the frustration.

Something we may want to consider before tagging new releases is to check which packages depend on the package we want to tag (perhaps using JuliaPkgDb!), check out master on our package, and run Pkg.test on the ones that depend on it.

andreasnoack · 2016-09-05T20:08:40Z

src/DataFrames.jl

@@ -16,6 +16,8 @@ using Reexport
 using GZip
 using SortingAlgorithms

+using FileIO  # remove after read_rda deprecation period


@ararslan Why is the dependency here FileIO and not RData?

FileIO recognizes the R data format by itself, so FileIO.load should work without RData. As for why the dependency is FileIO instead of RData, I had initially set it to RData but changed it to FileIO and I forget why. 😕

...but only if RData is loaded, right? So the dependency in REQUIRE should actually be RData? I was also a bit puzzled by the warning since it first points to RData and then mentions FileIO.

Yeah. I think the dependency should be on RData and the deprecation should use RData.load (which I also had before and changed). The deprecation should probably also just be simplified to, as Milan suggested, @deprecate read_rda(args...) RData.load(args...).

There's a discussion in #1031 why it should be FileIO.load() (because it accepts string names, whereas RData.load() method has different signature and is not to be called directly, it gets called by FileIO.load()). FileIO knows about .rda format and knows that its support is provided by RData package. So if no RData package is installed, the user would be asked to install it. By requiring FileIO we avoid circular references DataFrames -> RData -> DataFrames.

Aha! I knew there was a reason I changed it. Thanks, @alyst. Perhaps you might want to update your PR, @andreasnoack?

@alyst Thanks for the explanation. I've updated my PR.

* Remove all RDA functionality from the package * Depend on RData for the deprecation period * Use FileIO instead of RData

Remove all RDA functionality from the package

702e3bd

Depend on RData for the deprecation period

40fdf03

ararslan changed the title ~~[WIP] Remove all RDA functionality from the package~~ Remove all RDA functionality from the package Aug 21, 2016

nalimilan reviewed Aug 22, 2016
View reviewed changes

ararslan mentioned this pull request Aug 22, 2016

Precompile the module JuliaData/RData.jl#8

Closed

alyst reviewed Aug 24, 2016
View reviewed changes

alyst mentioned this pull request Aug 24, 2016

Register R data format JuliaIO/FileIO.jl#75

Merged

ararslan added 2 commits August 26, 2016 11:36

Use FileIO instead of RData

de77585

Oops, was still using RData

48dddbb

ararslan merged commit d2987d1 into master Aug 31, 2016

ararslan deleted the aa/goodbye_rda branch August 31, 2016 16:58

tlnagy mentioned this pull request Sep 5, 2016

RDatasets broken by recent deprecations in DataFrames #1054

Closed

andreasnoack reviewed Sep 5, 2016
View reviewed changes

andreasnoack mentioned this pull request Sep 6, 2016

Test failure on 0.5rc4 JuliaStats/StatsBase.jl#210

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all RDA functionality from the package #1031

Remove all RDA functionality from the package #1031

ararslan commented Aug 10, 2016 •

edited

nalimilan commented Aug 16, 2016

ararslan commented Aug 16, 2016 •

edited

ararslan commented Aug 17, 2016

nalimilan commented Aug 18, 2016

ararslan commented Aug 18, 2016

nalimilan Aug 22, 2016

nalimilan commented Aug 22, 2016

ararslan commented Aug 22, 2016

alyst Aug 24, 2016

nalimilan Aug 24, 2016

alyst Aug 24, 2016

dmbates commented Aug 24, 2016

nalimilan commented Aug 24, 2016

alyst commented Aug 26, 2016

ararslan commented Aug 30, 2016

ararslan commented Aug 30, 2016 •

edited

nalimilan commented Aug 31, 2016

ararslan commented Sep 5, 2016 •

edited

andreasnoack commented Sep 5, 2016

tlnagy commented Sep 5, 2016 •

edited

ararslan commented Sep 5, 2016

nalimilan commented Sep 5, 2016

andreasnoack commented Sep 5, 2016

tlnagy commented Sep 5, 2016

ararslan commented Sep 5, 2016

andreasnoack Sep 5, 2016

ararslan Sep 5, 2016

andreasnoack Sep 5, 2016 •

edited

ararslan Sep 5, 2016

alyst Sep 5, 2016 •

edited

ararslan Sep 5, 2016

andreasnoack Sep 6, 2016

		@@ -1,18 +1,23 @@
		using Base.depwarn
		import Base: @deprecate, depwarn

Remove all RDA functionality from the package #1031

Remove all RDA functionality from the package #1031

Conversation

ararslan commented Aug 10, 2016 • edited

nalimilan commented Aug 16, 2016

ararslan commented Aug 16, 2016 • edited

ararslan commented Aug 17, 2016

nalimilan commented Aug 18, 2016

ararslan commented Aug 18, 2016

Choose a reason for hiding this comment

nalimilan commented Aug 22, 2016

ararslan commented Aug 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmbates commented Aug 24, 2016

nalimilan commented Aug 24, 2016

alyst commented Aug 26, 2016

ararslan commented Aug 30, 2016

ararslan commented Aug 30, 2016 • edited

nalimilan commented Aug 31, 2016

ararslan commented Sep 5, 2016 • edited

andreasnoack commented Sep 5, 2016

tlnagy commented Sep 5, 2016 • edited

ararslan commented Sep 5, 2016

nalimilan commented Sep 5, 2016

andreasnoack commented Sep 5, 2016

tlnagy commented Sep 5, 2016

ararslan commented Sep 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreasnoack Sep 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyst Sep 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Aug 10, 2016 •

edited

ararslan commented Aug 16, 2016 •

edited

ararslan commented Aug 30, 2016 •

edited

ararslan commented Sep 5, 2016 •

edited

tlnagy commented Sep 5, 2016 •

edited

andreasnoack Sep 5, 2016 •

edited

alyst Sep 5, 2016 •

edited