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

add message that datatable.nomatch will be deprecated #3612

Merged
merged 4 commits into from
May 31, 2019
Merged

Conversation

jangorecki
Copy link
Member

@jangorecki jangorecki commented May 29, 2019

Closes #3585 (revised)
First step is to inform and communicate.

@codecov
Copy link

codecov bot commented May 29, 2019

Codecov Report

Merging #3612 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3612      +/-   ##
==========================================
+ Coverage   97.81%   97.81%   +<.01%     
==========================================
  Files          66       66              
  Lines       12927    12929       +2     
==========================================
+ Hits        12645    12647       +2     
  Misses        282      282
Impacted Files Coverage Δ
R/foverlaps.R 98.11% <100%> (+0.01%) ⬆️
R/data.table.R 97.74% <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 daf721a...88ac189. Read the comment docs.

@codecov
Copy link

codecov bot commented May 29, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3612   +/-   ##
=======================================
  Coverage   98.19%   98.19%           
=======================================
  Files          66       66           
  Lines       12922    12922           
=======================================
  Hits        12689    12689           
  Misses        233      233
Impacted Files Coverage Δ
R/foverlaps.R 98.1% <100%> (ø) ⬆️
R/data.table.R 97.74% <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 06a5ed6...21e12cd. Read the comment docs.

@mattdowle
Copy link
Member

Not sure if you saw my earlier comment here : #3578 (comment). This would allow us to not introduce the new datatable.natural option. And we'd deprecate datatable.nomatch too. There would be no need to document unsafe options, because there wouldn't be any unsafe options.

@jangorecki
Copy link
Member Author

Yes, but it is less trivial, and need message about nomatch option anyway. Deprecating nomatch will takes ages, this PR is first step. I can remove naturaljoin option altogether in this PR.

@mattdowle
Copy link
Member

mattdowle commented May 30, 2019

Yes deprecating nomatch option would need time and communication, and a message would be first step.

I can remove naturaljoin option altogether in this PR.

Do you mean you agree with my suggestion to use on=data.table to trigger natural join, or some other way to avoid the option?

@jangorecki
Copy link
Member Author

jangorecki commented May 30, 2019

I agree but I don't want to include that in this PR. It would be good to have it in separate PR. This would be API change and once users will start to use it will be difficult to amend changes to it.

@mattdowle mattdowle changed the title add message about unsafe options, #3585 add message that datatable.nomatch will be deprecated May 31, 2019
@mattdowle mattdowle added this to the 1.12.4 milestone May 31, 2019
@mattdowle mattdowle merged commit 3ae72ac into master May 31, 2019
@mattdowle mattdowle deleted the unsafe-opts branch May 31, 2019 04:27
@mattdowle mattdowle mentioned this pull request May 31, 2019
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.

deprecate datatable.nomatch; message first step
2 participants