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

deprecate datatable.nomatch; message first step #3585

Closed
jangorecki opened this issue May 23, 2019 · 4 comments · Fixed by #3612
Closed

deprecate datatable.nomatch; message first step #3585

jangorecki opened this issue May 23, 2019 · 4 comments · Fixed by #3612
Milestone

Comments

@jangorecki
Copy link
Member

jangorecki commented May 23, 2019

Originally posted by @mattdowle in #3578 (comment)

I fear it's not sufficient to just document this. There should be something more robust. I've been aware of this issue for some time and have not created any new options that affect behavior, only to manage migration temporarily and stated in news that those options will eventually be removed (e.g. datatable.old.unique.by.key). We only have 1 long-standing option which changes the results: datatable.nomatch. If a user changes that option for their own usage (because they prefer the nomatch=0L default), it could affect packages behavior. Even packages that didn't set any datatable options and were completely compliant with the text in this PR.

datatable.naturaljoin is now the 2nd one. Can we find an acceptable way for natural join to be explicit so we don't need the new option?

Here are all the options as of now in dev : https://github.com/Rdatatable/data.table/blob/master/R/onLoad.R#L44. Other than the those two (datatable.nomatch and datatable.naturaljoin) they all affect printing, optimization, or are temporary for migration.

This does mean we should try to remove datatable.nomatch too, or find a way for code inside packages to ignore its value.

@mattdowle
Copy link
Member

I don't see how this issue can be achieved, other than what I suggested: deprecate datatable.nomatch and remove data table.naturaljoin from dev.
How could options be 'less global'?

@jangorecki
Copy link
Member Author

jangorecki commented May 23, 2019

One idea is to ignore the option for calls from within packages, so only calls from non-package environment would the option. Ultimately it is best to deprecate and remove, agree.

@mattdowle
Copy link
Member

mattdowle commented May 23, 2019

That could work -- a simpler version of cedta() then I guess. Would we still need the whitelist? IIRC some packages make it look as-if (to cedta) that they are not a package, which is why we needed the whitelist for those packages. First-time package authors, if they have used datatable.nomatch=0L, may struggle when they find their code doesn't work as a package: they already struggle to just import when using the RStudio layer.

@jangorecki
Copy link
Member Author

That would be actually relatively complex, probably better is just to inform, and not harm any existing code that relies on that.

@mattdowle mattdowle added this to the 1.12.4 milestone May 31, 2019
@mattdowle mattdowle changed the title global options like nomatch could be less global when appropriate deprecate datatable.nomatch; message first step 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 a pull request may close this issue.

2 participants