Drop 0.4 and 0.7 fixes #385

yuyichao · 2017-07-23T00:21:28Z

Drop remaining 0.4 support code
Deprecate remaining 0.4 Compat bindings
Remove tests for 0.4 Compat
Do not run deprecated tests on 0.6
Recover one test that's incorrectly moved to deprecated (StringVector)
Add compat for at-nospecialize with tests
Add compat for read(..., String) with tests

This covers all the changes since it'll be really annoy to fix the above issues one by one......................

yuyichao · 2017-07-23T00:23:46Z

Also, there were a lot of 0.4 (and even 0.3!!!) compat code that was not removed previously so I added a rough version for all the rest that do not branch on version check. Also added some for the tests....

tkelman

new features need to be added to the readme

tkelman · 2017-07-23T02:55:56Z

test/runtests.jl

+
+@test read(IOBuffer("aaaa"), String) == "aaaa"
+@test contains(read(@__FILE__, String), "read(@__FILE__, String)")
+@test read(`$(Base.julia_cmd()) -e "println(:aaaa)"`, String) == "aaaa\n"


--startup-file=no

tkelman · 2017-07-23T02:58:54Z

Please don't delete so many tests. If it's not deprecated, leave it.

ararslan · 2017-07-23T03:01:55Z

What tests for non-deprecated functionality are being deleted here?

tkelman · 2017-07-23T03:05:59Z

most of the deletions in test/

yuyichao · 2017-07-23T03:09:04Z

If it's not deprecated, leave it.

The deleted tests are all unrelated to Compat. Keeping them would be like running the Base test suite again here.

tkelman · 2017-07-23T03:21:23Z

It's independent coverage of things and makes sure we don't break them.

yuyichao · 2017-07-23T03:33:39Z

It's independent coverage of things and makes sure we don't break them.

AFAICT they are all covered in base tests (in fact many of them are copied from base) so they are not independent coverage. These are additional runs but if you really want to catch that it's much better to setup a service for it. The test here are to test Compat.jl, not Base.

I also don't see why these test can help us catching breakage. For old releases, the test here might run more frequently than the base CI, but I don't see why running the tests here can catch anything. They increase CI time and the only reason I've seen a test fail on old version is for unrelated reasons (like binary download stops working). Adding tests won't help those.

For nightly, it'll run strictly less frequently than the base CI so it's pretty useless for catching base breakage. The tests here are also very primitive (they are all testing if the difference between versions are patched by the package and already always assume that the correct behavior is given by base). Examples include testing name existing and assuming they have the correct definition, testing against base implementation on old versions to make sure the new name has identical behavior. Tests like this are exactly what should be in the Compat test (it's testing the package) and should be removed when the corresponding Compat def is removed. It also basically means that the only breakage caught by these tests are intentional breakage in base and we'll need to fix the test here, instead of fixing base. Whenever a test has this kind of behavior, it's usually a signal that the test should not be there.

tkelman · 2017-07-23T08:00:54Z

should be removed when the corresponding Compat def is removed

I disagree with that. If it was once implemented as a feature here, then it should still be tested even if it's now a no-op, as people are still likely using it as if it isn't a no-op.

tkelman · 2017-07-23T08:04:21Z

src/deprecated.jl

@@ -143,6 +143,15 @@ end
 using .CompatCartesian
 export @ngenerate, @nsplat

+function primarytype(@nospecialize(t))


why is this in this file if it isn't deprecated?

What isn't deprecated? It's an internal function used only in deprecated code.

yuyichao · 2017-07-23T12:35:44Z

If it was once implemented as a feature here, then it should still be tested even if it's now a no-op, as people are still likely using it as if it isn't a no-op.

It was implemented as a patch on old versions and it was never implemented on 0.5+. It's not changed to no-op but simply removed and people never use it from Compat.

tkelman · 2017-07-23T12:42:46Z

People did get things from Compat on older versions. I don't see the benefit of deleting huge numbers of tests, they're harmless and arguably beneficial to leave in. It's not like they take all that long to run.

yuyichao · 2017-07-23T12:49:00Z

They are not harmless. They do take time to run and they are causing issues due to new changes in Base. I believe there's more harm to leave the in than the benefits since I can remember 4-5 times where an old test has to be disabled on master due to base changes but not a single time where a test breakage here indicates some unexpected base change.

yuyichao · 2017-07-23T12:50:05Z

People did get things from Compat on older versions.

older versions -> those are unsupported now.

tkelman · 2017-07-23T12:54:14Z

People have been writing code against these features though, as if they come from Compat.

Delete or disable them when they cause problems. They aren't causing a problem right now, so please don't delete them yet.

yuyichao · 2017-07-23T12:57:16Z

Delete or disable them when they cause problems.

That's exactly what useless tests are.

tkelman · 2017-07-23T13:03:46Z

I disagree. It doesn't hurt anything to run them.

yuyichao · 2017-07-23T13:11:23Z

A test is used to catch issues that cause it to fail. If when the test fail the fix is to remove the test then the test is useless.

And also, when you move one feature from one packge to another, you move the tests with it. We are basically moving the "features" (not really since they are never an Compat feature on 0.5+) from this package to Base so the tests should be moved to base as well (as was done a long time ago when the correspondign base features were added.).

yuyichao · 2017-07-23T13:14:02Z

It doesn't hurt anything to run them.

Well, not run them. It hurts (waste a lot of time) to maintain them.

tkelman · 2017-07-23T13:24:22Z

It takes less time to incrementally fix the handful that cause problems over time than the substantial review burden right now of deleting over a thousand lines of tests to verify that you aren't deleting anything that shouldn't be. The default with tests is that more is better and it's easier to leave them alone.

yuyichao · 2017-07-23T13:33:08Z

It takes less time to incrementally fix the handful that cause problems over time than the substantial review burden right now of deleting hundreds of lines of tests to verify that you aren't deleting anything that shouldn't be.

I disagree. I'll certain choose to review a PR like this than to keep maintaining old tests overtime, which I did and it's a waste of time. The waste of time was necessary when we still supported the old versions but not when we don't. The loc removed in tests here isn't really more than #372 (or at least what should have been in it since it missed many 0.3 and 0.4 compat defs), so I don't think it's harder than that PR to verify that the removal are correct. FWIW, I'm not a huge fan of dropping 0.4 support here but I do think that when we do, the code and the tests for them should be remove together and that'll actually make reviewing easier.

The default with tests is that more is better and it's easier to leave them alone.

That's certainly not true. Bad and useless tests make things worse. Again, tests are there to catch breakage. But if the expectation of the tests are that their failure is always a signal that the tests themselves need to be fixed or removed than they don't make anything better and they make things worse by increasing the maintenance effort, exactly the opposite what they should do.

yuyichao · 2017-07-23T14:03:15Z

I totally agree that this needs to be checked and that's the point of PR review. For checking myself and to make reviewing easier. I've created a gist for all the tests that are removed (not including moved ones) here https://gist.github.com/yuyichao/f7243e25250b54e55bbd5d102489d264. The file should pass on 0.5 and 0.6 without any error or warnings. It should be easy to verify if the file is an accurate list of tests removed and if the few compat defs are needed are tested in current tests. Sys.*, take!(::IOBuffer) in runtests.jl, *String aliases in deprecated.jl. I do think this can be used in the future as a required reviewing procedure when we remove def and tests for old compat definitions.

tkelman · 2017-07-25T13:51:54Z

I'm still in the process of reviewing this, would like a bit more time if you're not in a hurry.

#372 intentionally didn't remove tests, it removed code under src that was dead not tests that would now unconditionally be checking Base behavior. The Compat test cases are if anything a bit less likely than a normal equivalent amount of code to hit breakage on nightly Julia (since they're things that have changed at least once before in recent memory).

yuyichao · 2017-07-25T14:16:19Z

Right, those tests are NOT removed. It's just the easiest to remember cases of compat test being broken due to base changes. Their use in other tests needs to be fixed (many of which are for old compat that are now removed in this PR) and their test for old behaviors are conditioned on old julia versions, which are either removed or moved to deprecated.

stevengj · 2017-07-25T14:20:36Z

+1 for removing tests for features that Compat no longer provides. The point of those tests is to test the Compat implementation, not to test the Base implementation.

Keeping them just seems like it invites maintenance headaches.

musm · 2017-07-28T17:47:28Z

Any chance this can be merged soon? Looking forward to the readstring compat fix here

tkelman · 2017-07-28T17:50:19Z

Still looking over this, should finish over the weekend

musm · 2017-07-28T17:51:47Z

Thanks, no strong rush however.

* Drop remaining 0.4 support code * Deprecate remaining 0.4 Compat bindings * Remove tests for 0.4 Compat * Do not run deprecated tests on 0.6 * Recover one test that's incorrectly moved to deprecated (StringVector) * Add compat for at-nospecialize with tests * Add compat for `read(..., String)` with tests

ararslan · 2017-08-04T19:44:45Z

Bump

yuyichao · 2017-08-04T19:49:16Z

I don't see what's time consuming about reviewing given the gist linked above #385 (comment) since there's a clear rule of when the code can be removed (i.e. if they can run without Compat.jl being loaded) so merge.

tkelman · 2017-08-04T20:04:20Z

sorry had been looking at other things instead of this

tkelman · 2017-08-04T20:06:16Z

src/Compat.jl

 ex = Expr(:curly, map(a -> isexpr(a, :call, 2) && a.args[1] == :(<:) ?
 :($TypeVar($(QuoteNode(gensym(:T))), $(a.args[2]), false)) :
 isexpr(a, :call, 2) && a.args[1] == :(>:) ?
 :($TypeVar($(QuoteNode(gensym(:T))), $(a.args[2]), $Any, false)) : a,
 ex.args)...)
 end
- elseif ex.head === :macrocall


why isn't this needed any more?

f isn't used, It's a piece of code that the last PR failed to remove.

tkelman · 2017-08-04T20:08:47Z

src/Compat.jl

- end
-end
-
-if !isdefined(Base, :Threads)


this is still needed for a no-threads build, please put it back

No it's not. The module is always available.

tkelman · 2017-08-04T20:14:30Z

src/deprecated.jl

@@ -160,13 +169,22 @@ end
 Base.@deprecate_binding KERNEL Sys.KERNEL
 Base.@deprecate_binding UTF8String Core.String
 Base.@deprecate_binding ASCIIString Core.String
+ Base.@deprecate_binding unsafe_convert Base.unsafe_convert
+ Base.@deprecate_binding remote_do Base.remote_do


also remotecall, remotecall_fetch, remotecall_wait

No those are exported symbols.

tkelman · 2017-08-04T20:15:44Z

src/deprecated.jl

+ Base.@deprecate_binding AsyncCondition Base.AsyncCondition
+ Base.@deprecate_binding promote_eltype_op Base.promote_eltype_op
+ @eval Base.@deprecate_binding $(Symbol("@irrational")) Base.$(Symbol("@irrational"))
+ @eval Base.@deprecate_binding $(Symbol("@blasfunc")) Base.LinAlg.BLAS.$(Symbol("@blasfunc"))
 else


missing srand, rand, rand!

Same as above.

tkelman · 2017-08-04T20:24:34Z

test/runtests.jl

-
-# qr, qrfact, qrfact!
-let A = [1.0 2.0; 3.0 4.0]
- Q, R = qr(A, valfalse)


Compat should be backporting the 0.7 version of this

It isn't currently. If it was backported then yes, a similar test should be added in that pr.

then better to leave this if it's going to need to be added back

No. We should keep tests that's testing what's in the package, not what could have been in or what was in the package. This procedure makes it much more clear what is being tested and what it's testing.

Also, no one have asked for such a feature so it's not 100% certain if one will be made. If none was made, it will make it much harder, like what I have to do this time, to figure out why the test is here and at the same time harder for the next version dropping PR to be reviewed. If one was added, explicitly adding it back later also make it much more clear to the reviewer what's actually being tested, without having to dig through the history to figure out if the test wasn't being incorrectly re-proposed.

It's not like the test can't be found anymore, they can be retried easily. It's also not like if the test is kept here it can be directly used, it'll have to be modified to actually test what the PR want.

Val() was added in #381, the apis that use it certainly will be as people get to fixing 0.7 depwarns

people don't go through deleted code when writing new tests

There was many cases where deprecated API's are not added in Compat.

In any case, I'm just listing both cases and as I mentioned above, even if you are 100% sure the Compat support will be added, without the context in this thread, it will actually take more review effort and won't really help the one writing the PR. For the PR author, it'll be easier to just check the Base tests changed in the Val Base PR JuliaLang/julia#22475, instead of going through the old tests and figure out if they can be modified for this purpose. For reviewer, it'll also be much harder to figure out if the repurpose of the test is correct. It won't be if we haven't drop 0.4 support yet so the reviewer needs to check that whatever this test is doing is already supported by base julia 0.5, doing that check in a batch with a single script containing deleted tests is the whole point why reviewing the deletion here is easier than deleting one at a time as we go.

tkelman · 2017-08-05T11:57:51Z

src/Compat.jl

- end
-end
-
-function rewrite_dict(ex)


Lint.jl was using this

AFAICT it was just using this as test code. Deprecating this won't even help.

* new style call overloading (added in #181, removed in #385) * `get(io, s false)` (added in #212, #215, #225, removed in #385) * `.=` (added in #292 and #316, removed in #372)

* Remove at-compat for type aliases Was added in #326, now obsolete as no longer required on minimum supported Julia version 0.6. * Remove at-compat for Nullable construction Was added in #287, now obsolete as no longer required on minimum supported Julia version 0.6. * Remove at-compat for Foo{<:Bar} sugar Was added in #317 (and #336), now obsolete as no longer required on minimum supported Julia version 0.6. * Remove at-compat for index styles Was added in #329, now obsolete as no longer required on minimum supported Julia version 0.6. * Remove at-compat for type declarations Was added in #325, now obsolete as no longer required on minimum supported Julia version 0.6. * Remove unused at-compat helper functions * Remove README entries for removed at-compat functionality * new style call overloading (added in #181, removed in #385) * `get(io, s false)` (added in #212, #215, #225, removed in #385) * `.=` (added in #292 and #316, removed in #372) * Remove `Compat.collect(A)` Was added in #350 and #351, now obsolete as no longer required on minimum supported Julia version 0.6.

* Bump required Julia version to 1.0 * Remove compatibility support code for: * `at-__MODULE__` (from #363) * `devnull`, `stdin`, `stdout`, and `stderr` from #499 * `at-nospecialize` (from #385 and #409) * `isabstracttype` and `isconcretetype` (from #477) * `invokelatest` from #424 * array-like access to `Cmd` from #379 * `Val(n)` and `ntuple`/`reshape` with `Val` from #381 and #399 * `logdet(::Any)` fallback from #382 * `chol(::UniformScaling)` from #382 * `pushfirst!`, `popfirst!` from #444 * `fieldcount` from #386 * `read(obj, ::Type{String})` from #385 and #580 * `InexactError`, `DomainError`, and `OverflowError` constructors from #393 * `corrected` kw arg to `cov` from #401 * `adjoint` from #401 * `partialsort` from #401 * `pairs` from #428 * `AbstractRange` from #400 * `rtoldefault` from #401 * `Dates.Period` rounding from #462 * `IterativeEigensolvers` from #435 * `occursin` from #520 * `Char` concatenation from #406 * `BitSet` from #407 * `diagm` and `spdiagm` with pairs from #408 * `Array` c'tors from `UniformScaling` from #412 and #438 * `IOContext` ctor taking pairs from #427 * `undef` from #417 and #514 * `get` on `ENV` from #430 * `ComplexF...` from #431 * `tr` from #614 * `textwidth` from #644 * `isnumeric` from #543 * `AbstractDict` from #435 * `axes` #435 and #442 * `Nothing` and `Cvoid` from #435 * `Compat.SuiteSparse` from #435 * `invpermute!` from #445 * `replace` with a pair from #445 * `copyto!` from #448 * `contains` from #452 * `CartesianIndices` and `LinearIndices` from #446, #455, and #524 * `findall` from #466 (and #467). * `argmin` and `argmax` from #470, #472, and #622 * `parentmodule` from #461 * `codeunits` from #474 * `nameof` from #471 * `GC` from #477 * `AbstractDisplay` from #482 * `bytesavailable` from #483 * `firstindex` and `lastindex` from #480 and #494 * `printstyled` from #481 * `hasmethod` from #486 * `objectid` from #486 * `Compat.find*` from #484 and #513 * `repr` and `showable` from #497 * `Compat.names` from #493 and #505 * `Compat.round` and friends #500, #530, and #537 * `IOBuffer` from #501 and #504 * `range` with kw args and `LinRange` from #511 * `cp` and `mv` from #512 * `indexin` from #515 * `isuppercase` and friends from #516 * `dims` and `init` kwargs from #518, #528, #590, #592, and #613 * `selectdim` from #522 and #531 * `repeat` from #625 * `fetch(::Task)` from #549 * `isletter` from #542 * `isbitstype` from #560 * `at-cfunction` from #553 and #566 * `codeunit` and `thisind` and friends from #573 * `something` from #562 * `permutedims` from #582 * `atan` from #574 * `split` and `rsplit` from #572 * `mapslices` from #588 * `floatmin` and `floatmax` from #607 * `dropdims` from #618 * required keyword arguments from #586 * `CartesianRange` in `at-compat` from #377 * `finalizer` from #416 * `readline`, `eachline`, and `readuntil` from #477, #541, and #575 * curried `isequal`, `==`, and `in` from #517 * `Some` from #435 and #563 * `at-warn` and friends from #458 * Remove old deprecations * Deprecate: * `Compat.Sockets` from #545 and #594 * `TypeUtils` from #304 * `macros_have_sourceloc` from #355 * `Compat.Sys` from #380, #433, and #552 * `Compat.MathConstants` from #401 * `Compat.Test`, `Compat.SharedArrays`, `Compat.Mmap`, and `Compat.DelimitedFiles` from #404 * `Compat.Dates` from #413 * `Compat.Libdl` from #465 (and #467) * `AbstractDateTime` from #443 * `Compat.Printf` from #435 * `Compat.LinearAlgebra` from #463 * `Compat.SparseArrays` from #459 * `Compat.Random` from #460, #601, and #647 * `Compat.Markdown` from #492 * `Compat.REPL` from #469 * `Compat.Serialization` from #473 * `Compat.Statistics` from #583 * `Fix2` from #517 * `Compat.Base64` from #418 * `Compat.Unicode` from #432 and #507 * `notnothing` from #435 and #563 * `Compat.IteratorSize` and `Compat.IteratorEltype` from #451 * `enable_debug(::Bool)` from #458 * `Compat.Distributed` from #477 * `Compat.Pkg` from #485 * `Compat.InteractiveUtils` from #485 * `Compat.LibGit2` from #487 * `Compat.UUIDs` from #490 * `Compat.qr` from #534 * `Compat.rmul!` from #546 * `Compat.norm` abd friends from #577 * Remove obsolete README entry, missed in #385 * Remove obsolete tests (e.g. missed in #372) * Remove obsolete `VERSION` conditionals and some minor clean-up

yuyichao force-pushed the yyc/0.7 branch from d0fcd88 to a4654bb Compare July 23, 2017 00:54

ararslan approved these changes Jul 23, 2017

View reviewed changes

tkelman suggested changes Jul 23, 2017

View reviewed changes

tkelman reviewed Jul 23, 2017

View reviewed changes

yuyichao force-pushed the yyc/0.7 branch 2 times, most recently from 58b867a to 165600e Compare July 23, 2017 03:15

tkelman reviewed Jul 23, 2017

View reviewed changes

yuyichao force-pushed the yyc/0.7 branch from 165600e to d4e5ffd Compare July 23, 2017 13:25

yuyichao force-pushed the yyc/0.7 branch from d4e5ffd to ed9e466 Compare July 23, 2017 13:57

ararslan force-pushed the yyc/0.7 branch from ed9e466 to 003dc9d Compare July 27, 2017 17:34

yuyichao force-pushed the yyc/0.7 branch from 003dc9d to f705a0f Compare July 30, 2017 15:07

yuyichao merged commit 8281370 into master Aug 4, 2017

yuyichao deleted the yyc/0.7 branch August 4, 2017 19:49

tkelman reviewed Aug 4, 2017

View reviewed changes

tkelman reviewed Aug 5, 2017

View reviewed changes

ararslan mentioned this pull request Jun 19, 2018

Allow any AbstractCmd in read #580

Merged

martinholters added a commit that referenced this pull request Sep 29, 2019

Drop compat code for at-nospecialize (from #385 and #409)

306bf6e

martinholters added a commit that referenced this pull request Sep 29, 2019

Drop compat code for at-nospecialize (from #385 and #409)

2dbd40b

martinholters added a commit that referenced this pull request Oct 1, 2019

Drop compat code for read(obj, ::Type{String}) from #385 and #580

21a3028

martinholters added a commit that referenced this pull request Oct 8, 2019

Remove obsolete README entry, missed in #385

5f72a77

martinholters added a commit that referenced this pull request Oct 8, 2019

Drop compat code for at-nospecialize (from #385 and #409)

0ba707d

martinholters added a commit that referenced this pull request Oct 8, 2019

Drop compat code for read(obj, ::Type{String}) from #385 and #580

8759c33

martinholters added a commit that referenced this pull request Oct 8, 2019

Remove obsolete README entry, missed in #385

bc6bd70

Drop 0.4 and 0.7 fixes #385

Drop 0.4 and 0.7 fixes #385

Conversation

yuyichao commented Jul 23, 2017 • edited Loading

yuyichao commented Jul 23, 2017

tkelman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Jul 23, 2017

ararslan commented Jul 23, 2017

tkelman commented Jul 23, 2017

yuyichao commented Jul 23, 2017

tkelman commented Jul 23, 2017

yuyichao commented Jul 23, 2017 • edited Loading

tkelman commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuyichao commented Jul 23, 2017

tkelman commented Jul 23, 2017

yuyichao commented Jul 23, 2017

yuyichao commented Jul 23, 2017

tkelman commented Jul 23, 2017

yuyichao commented Jul 23, 2017

tkelman commented Jul 23, 2017

yuyichao commented Jul 23, 2017

yuyichao commented Jul 23, 2017

tkelman commented Jul 23, 2017 • edited Loading

yuyichao commented Jul 23, 2017 • edited Loading

yuyichao commented Jul 23, 2017

tkelman commented Jul 25, 2017

yuyichao commented Jul 25, 2017

stevengj commented Jul 25, 2017

musm commented Jul 28, 2017

tkelman commented Jul 28, 2017

musm commented Jul 28, 2017

ararslan commented Aug 4, 2017

yuyichao commented Aug 4, 2017

tkelman commented Aug 4, 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

Choose a reason for hiding this comment

yuyichao Aug 4, 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

yuyichao commented Jul 23, 2017 •

edited

Loading

yuyichao commented Jul 23, 2017 •

edited

Loading

tkelman commented Jul 23, 2017 •

edited

Loading

yuyichao commented Jul 23, 2017 •

edited

Loading

tkelman commented Aug 4, 2017 •

edited

Loading

yuyichao Aug 4, 2017 •

edited

Loading