Skip to content

Restore reshape2 redirection (with warning)#3763

Merged
mattdowle merged 3 commits intomasterfrom
restore_reshape2
Aug 26, 2019
Merged

Restore reshape2 redirection (with warning)#3763
mattdowle merged 3 commits intomasterfrom
restore_reshape2

Conversation

@MichaelChirico
Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico commented Aug 13, 2019

Closes #3633
follow-up to #3577

I looked at the reshape2 list method for melt, it's quite unintuitive to me, so I wouldn't re-implement it as-is to data.table. We might want to add melt.list in the future but I don't think it would look like reshape2's (which relies on attr of the list at various levels?)

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 13, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3763      +/-   ##
==========================================
+ Coverage   99.41%   99.41%   +<.01%     
==========================================
  Files          71       71              
  Lines       13155    13197      +42     
==========================================
+ Hits        13078    13120      +42     
  Misses         77       77
Impacted Files Coverage Δ
R/fmelt.R 100% <100%> (ø) ⬆️
R/fcast.R 100% <100%> (ø) ⬆️
src/assign.c 100% <0%> (ø) ⬆️
src/gsumm.c 100% <0%> (ø) ⬆️
src/fifelse.c 100% <0%> (ø) ⬆️
src/init.c 100% <0%> (ø) ⬆️
R/between.R 100% <0%> (ø) ⬆️
R/groupingsets.R 100% <0%> (ø) ⬆️
src/utils.c 100% <0%> (ø) ⬆️
R/data.table.R 100% <0%> (ø) ⬆️
... and 1 more

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 a2e7f5e...60282af. Read the comment docs.

Comment thread R/fcast.R
Comment thread R/fcast.R Outdated
# reshape2::dcast is not generic so we have to call it explicitly. See comments at the top of fmelt.R too.
ns = tryCatch(getNamespace("reshape2"), error=function(e)
stop("The dcast generic in data.table has been passed a ",class(data)[1L]," (not a data.table) but the reshape2 package is not installed to process this type. Please either install reshape2 and try again, or pass a data.table to dcast instead."))
warning("The dcast generic in data.table has been passed a ", class(data)[1L], " and will attempt to redirect to the relevant reshape2 method; please note that reshape2 is deprecated, and this redirection is now deprecated as well.")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should propose a solution in this warning message. Something like

if you attach both packages use melt/dcast prefixing expected namespace. Or move to using data.table versions, and report if any features you are using are missing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

and stating when more or less it will be deprecated (thus might result in breaking code) might be useful too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Have updated both the warning and the error, please have a look

Comment thread R/fmelt.R
# nocov start
} else {
# nocov start
data_name = deparse(substitute(data))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would avoid deparse if not really needed, as here. For a very long symbol names it can cause problems. The gain from using that here is really minor.

@mattdowle mattdowle changed the title Closes #3633 -- restore reshape2 redirection (with warning) Restore reshape2 redirection (with warning) Aug 26, 2019
@mattdowle mattdowle added this to the 1.12.4 milestone Aug 26, 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.

Handling how data.table::melt is not passing to reshape2::melt.list method anymore.

3 participants