Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean out pre-0.4 cruft #323

Merged
merged 3 commits into from
Feb 13, 2017
Merged

Clean out pre-0.4 cruft #323

merged 3 commits into from
Feb 13, 2017

Conversation

timholy
Copy link
Member

@timholy timholy commented Feb 10, 2017

Puts Compat on a bit of a diet. I wasn't sure whether we should deprecate old functionality (do we need CompatCompat.jl?), but I did so for safety. I am also not sure whether we want to run tests of deprecated functionality, but again included it for safety.

@timholy
Copy link
Member Author

timholy commented Feb 10, 2017

Looks like this will trigger warnings in StatsBase (re @ngenerate).

@timholy
Copy link
Member Author

timholy commented Feb 11, 2017

I modified the deprecation warning for @ngenerate to urge people to try to switch to tuples/CartesianIndex rather than running straight for @generated function (though I listed that as an option).

Among the packages installed on my machine, the other remaining consumer of @ngenerate is DataArrays.

@tkelman
Copy link
Contributor

tkelman commented Feb 11, 2017

I'd like to avoid introducing deprecation warnings on Julia 0.4, since many packages aren't making new releases there. When we drop support for 0.4 would be a good time to do this, though this likely should be one of the last packages to do that.

@timholy
Copy link
Member Author

timholy commented Feb 11, 2017

Very reasonable choice. I commented out the depwarns and renamed the files to-be-deprecated.jl. Once this is merged you/I should file an issue to remind us to uncomment the depwarns once 0.4 support is dropped.

end
export Libc, Libdl
else
import Base: Libc, Libdl
Copy link
Contributor

Choose a reason for hiding this comment

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

should be kept, or Compat.Libc moved to to-be-deprecated

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessary.

julia> Compat.Libc
Base.Libc

include("linspace.jl")
const linspace = LazyLinSpace.linspace
else
const linspace = Base.linspace
Copy link
Contributor

Choose a reason for hiding this comment

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

likewise should keep Compat.linspace available, possible future deprecation candidate

Copy link
Member Author

Choose a reason for hiding this comment

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

julia> Compat.linspace == Base.linspace
true

@test Compat.@Dict(1 => 1) == d
@test Compat.@Dict(1 => v) == d

@test typeof(@compat(Dict(1 => 1))) == Dict{Int,Int}
Copy link
Contributor

Choose a reason for hiding this comment

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

won't this be a no-op rather than a depwarn?

Copy link
Member Author

Choose a reason for hiding this comment

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

They won't trigger a deprecation warning, but conversely the tests aren't required since Compat doesn't do anything to Dict syntax anymore. No point hanging on to no-op tests.

end

if VERSION < v"0.4.0-dev+5322"
include("timer.jl")
Copy link
Contributor

Choose a reason for hiding this comment

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

delete this file then i guess?

@timholy
Copy link
Member Author

timholy commented Feb 11, 2017

timer.jl deleted.

@timholy
Copy link
Member Author

timholy commented Feb 13, 2017

Rebased. I'll merge later today unless I hear otherwise.

@yuyichao
Copy link
Contributor

Maybe merge after #326?

@timholy
Copy link
Member Author

timholy commented Feb 13, 2017

That one is unquestionably of higher priority. This is just one of those PRs that shouldn't linger too long, since it's a magnet for merge conflicts.

@yuyichao
Copy link
Contributor

Actually I thought this removes 0.4 support which is not the case so nvm. I'll rebase this one if I want to merge #326 (probably early afternoon) before you merge this one.

@tkelman
Copy link
Contributor

tkelman commented Feb 13, 2017

Is (most of) this code hurting anything by being here but inactive?

@yuyichao
Copy link
Contributor

It shouldn't hurt anything though it should be done right after 0.3 support is dropped...

calltypes[k] = v
end
elseif VERSION < v"0.5.0-dev+3831"
import Base.unsafe_convert
Copy link
Contributor

Choose a reason for hiding this comment

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

So what'll be the timeline for deprecating and removing this? (Assuming we don't want people to use Compat.unsafe_convert for ever, might also apply to a few other places).

I can see the argument of not giving new depwarn on 0.4 though waiting longer doesn't help since we always drop a version here only when virtually no one is using it anymore. Should we do that on the same release that drop 0.4 support assuming that's when 0.5 is still widely tested?

@tkelman
Copy link
Contributor

tkelman commented Feb 13, 2017

should be done right after 0.3 support is dropped

That was a while ago, and I argued against it at the time since leaving it in makes the possibility of restoring 0.3 support including (some of?) the latest features a little simpler, if a need ever arises for that. Code deletions could be reverted and people could use branches for such things, but if it's harmless to keep it...

@timholy
Copy link
Member Author

timholy commented Feb 13, 2017

Proximal motivation was that I was entertaining the possibility of putting in the new StepRangeLen and LinSpace. But there was some old linspace Compat stuff, and I didn't want to mess with trying to potentially have two versions of that loaded.

@tkelman
Copy link
Contributor

tkelman commented Feb 13, 2017

So the old pre-0.4 linspace is hurting stuff by being here, I'm fine with deleting that if it's in your way.

@timholy
Copy link
Member Author

timholy commented Feb 13, 2017

I should confess that I can't promise to get to this immediately. We could merge this when I'm sure I'll have time.

@TotalVerb
Copy link
Contributor

There has been an important change since 0.3 support was dropped: with 0.6 on the horizon, I think that the chances of ever needing to backport anything to 0.3 are now very low. So deleting the code makes sense to me.

@timholy
Copy link
Member Author

timholy commented Feb 13, 2017

OK, let's do it now just so there aren't rebase conflicts in the future.

@timholy timholy merged commit 44207e4 into master Feb 13, 2017
@timholy timholy deleted the teh/cruft branch February 13, 2017 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants