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

Modernize ?setkey; and tackle long-deprecated functions #3399

Merged
merged 12 commits into from
Mar 2, 2019

Conversation

MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Feb 13, 2019

Towards #2572

git blame on setkey.R shows set2key hasn't been changed in 3 years, and key<- in 4 years.

I think we can safely delete these finally, and strip all but the historical reference from the documentation; that's done in this PR

@jangorecki
Copy link
Member

why not keep it for 2 more years?

@codecov
Copy link

codecov bot commented Feb 14, 2019

Codecov Report

Merging #3399 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3399      +/-   ##
==========================================
- Coverage   95.11%   95.11%   -0.01%     
==========================================
  Files          65       65              
  Lines       12118    12113       -5     
==========================================
- Hits        11526    11521       -5     
  Misses        592      592
Impacted Files Coverage Δ
R/setkey.R 98.22% <ø> (-0.04%) ⬇️

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 99f6376...d09640d. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 14, 2019

Codecov Report

Merging #3399 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3399   +/-   ##
=======================================
  Coverage   95.09%   95.09%           
=======================================
  Files          65       65           
  Lines       12301    12301           
=======================================
  Hits        11698    11698           
  Misses        603      603
Impacted Files Coverage Δ
R/duplicated.R 94.59% <ø> (ø) ⬆️
R/setkey.R 98.26% <100%> (ø) ⬆️
R/data.table.R 97.47% <100%> (ø) ⬆️
src/assign.c 94.89% <100%> (ø) ⬆️

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 8105bae...2bf17ac. Read the comment docs.

@HughParsonage
Copy link
Member

why not keep it for 2 more years?

Indeed, why not keep it indefinitely? One thing I notice helping out people with R is that there is a very large tail of users who might use R once or twice a year and there are many users who use it only once. All they know is 'this is an R script which does X, run it if you want X'. They have no hope of recovering from errors like these.

I think intervening with an new error is good if it avoids unexpected behaviour (i.e. a bug), but with everything else let sleeping dogs lie.

This is especially so with key(x)<- because the warning message says 'You can safely ignore this warning...'. We could remove that but I think it's pretty costless to just leave it.

@jangorecki
Copy link
Member

jangorecki commented Feb 15, 2019

Base R is known for its backward compatibility. Why not to align to such strategy? Think about blog posts from where people will just try to copy paste code. StackOoverflow we could easily find and update, but there is not really need for that. Warnings are OK because they will still produce final outcome.
Removing from manual should be fine, but we would need to move those manuals to some deprecated.Rd.

@MichaelChirico
Copy link
Member Author

I think we should move key<- to an error first, yes; will redact and put a deprecation error.

But for set2key family, from ?Deprecated:

When an object is about to be removed from R it is first deprecated and should include a call to .Deprecated.

(emphasis mine)

Deprecation means eventually that functionality will be removed completely. I think 3 years is plenty "eventual".

@jangorecki
Copy link
Member

good to put .Deprecated call there, but there is no real reason to remove it after 3 years.

@mattdowle
Copy link
Member

mattdowle commented Mar 1, 2019

Good idea to clear these things up. We should clear the technical debt at some point as it does add to the maintenance burden. I don't know how well key<- works (or not) these days with latest versions of R. Best to move towards eventual removal with good communication.

The past communication in NEWS should drive these things.

From v1.11.0 on CRAN May 2018 (less than a year ago) :

  1. As set2key() and key2() have been warning since v1.9.8 (Nov 2016), their warnings have now been upgraded to errors. Note that when they were introduced in version 1.9.4 (Oct 2014) they were marked as 'experimental' in NEWS item 4. They will be removed in one year.
    Was warning :
    set2key() will be deprecated in the next relase. Please use setindex() instead.
    Now error:
    set2key() is now deprecated. Please use setindex() instead.

One year from then would be May 2019. So now is too early for that reason.

@mattdowle mattdowle added this to the 1.12.2 milestone Mar 1, 2019
@mattdowle
Copy link
Member

@MichaelChirico Just been going through ?setkey. Yep : this was sorely needed! Good call.

@MichaelChirico MichaelChirico mentioned this pull request Mar 2, 2019
51 tasks
@mattdowle mattdowle changed the title Modernize ?setkey; delete long-deprecated functions; part of #2572 Modernize ?setkey; and tackle long-deprecated functions Mar 2, 2019
@mattdowle mattdowle merged commit e192793 into master Mar 2, 2019
@mattdowle mattdowle deleted the setkey_man_update branch March 2, 2019 02:36
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