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

Remove reshape2 suggest #3081

Merged
merged 3 commits into from
Sep 27, 2018
Merged

Remove reshape2 suggest #3081

merged 3 commits into from
Sep 27, 2018

Conversation

mattdowle
Copy link
Member

Closes #3080
Completes final item in #2740

Added comments to the top of fmelt.c as to why we don't import reshape2 (R dependency inheritance)

melt is generic in reshape2 so I'm not sure why we were explicitly calling it

dcast dispatches non-data.table to non-generic reshape2::dcast the same as before, but with a more helpful message if reshape2 is not installed.

Tidied NAMESPACE. There were a few methods explicitly exported which didn't need to be since S3method already exports the method. This leaves just one use of the new delayed S3method registration, for xts.

The as.Date method has another twist in that zoo::as.Date masks base::as.Date and they are both generic.

@mattdowle mattdowle added this to the 1.11.8 milestone Sep 27, 2018
@codecov
Copy link

codecov bot commented Sep 27, 2018

Codecov Report

Merging #3081 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3081      +/-   ##
==========================================
+ Coverage   90.86%   90.88%   +0.02%     
==========================================
  Files          61       61              
  Lines       11808    11804       -4     
==========================================
- Hits        10729    10728       -1     
+ Misses       1079     1076       -3
Impacted Files Coverage Δ
R/fcast.R 83.79% <ø> (+0.92%) ⬆️
R/fmelt.R 95% <100%> (+2.14%) ⬆️

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 125416c...a32c5e5. Read the comment docs.

@mattdowle mattdowle changed the title Removed reshape2 suggest Remove reshape2 suggest Sep 27, 2018
@mattdowle mattdowle merged commit 0acf022 into master Sep 27, 2018
@mattdowle mattdowle deleted the remove_reshape2 branch September 27, 2018 23:47
S3method(anyDuplicated, data.table)

export(split.data.table)
Copy link
Member

Choose a reason for hiding this comment

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

is this really required? this was exported in 1.11.6 #2920 as per user request in a comment somewhere. As it is very recent it won't break much code, but NEWS entry would be useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

A comment next to it explaining why and when exported would have been good because it's not proper to do so in addition to the S3method(split, data.table) which is still there. Should we consider exporting all methods for base generics as well as registering them?

Copy link
Member Author

@mattdowle mattdowle Sep 28, 2018

Choose a reason for hiding this comment

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

Added a news item to 1.11.8 : #3083

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.

2 participants