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

export split.data.table #2920

Closed
jangorecki opened this issue Jun 2, 2018 · 1 comment · Fixed by #2921
Closed

export split.data.table #2920

jangorecki opened this issue Jun 2, 2018 · 1 comment · Fixed by #2921
Milestone

Comments

@jangorecki
Copy link
Member

jangorecki commented Jun 2, 2018

It might happened that users miss that we have split method just because we exported it as a method only, and not as function directly, thus split.data.table cannot be accessed without :::.
There is no harm in exporting that.

@mattdowle
Copy link
Member

mattdowle commented Sep 28, 2018

thus split.data.table cannot be accessed without :::

that doesn't seem quite right because split(DT) dispatches to split.data.table if DT is a data.table and is the normal way to access it. It was already done properly before using S3method(split, data.table) in NAMESPACE. Exporting the method explicitly (i.e. the export(split.data.table) that was added) means that users can then call the method directly and they're not supposed to do that. They're supposed to leave it to the generic: split(DT). In this case the split() generic is in base so we're even more supposed to respect it and not bypass it. The harm in exporting methods explicitly is that they might start to diverge/become incompatible with that generic. Since when a user calls the exported split.data.table method, it's not really a method anymore, that's the method being used directly as a function. It may have come about because we export dcast.data.table. But that's because dcast is not generic in reshape2 and so if reshape2 was higher on the search path, dcast(DT) would call the reshape2 one which wouldn't do any dispatch.

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