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

as.data.table.list centralized #3471

Merged
merged 17 commits into from Jun 14, 2019
Merged

as.data.table.list centralized #3471

merged 17 commits into from Jun 14, 2019

Conversation

mattdowle
Copy link
Member

@mattdowle mattdowle commented Mar 26, 2019

data.table and as.data.table.list were very similar. Moved the logic into as.data.table.list and called that from data.table().
Original motivation was to unpack columns from a list of several data.frame which arose via jsonlite here: #3369 (comment). New tests 2057.* mimic that case and the test.R file from the .zip in that issue passes with this PR.
Another motivation was to get the common code into one place so it can then be ported to C eventually. C level not for speed but to use MAYBE_REFERENCED to only copy when needed.

Test 1484 strengthened to check fix for #842 still fixed (branch using point() removed).
CcopyNamedInList removed as as.data.table.list now does that.
cbind(DT, list(...)) now creates list column so tests 1613.571-3 needed their list() wrappers removed. Those list() around factor(1) in those 3 tests seemed wrong anyway.

  • revisit #3480. setDT(list) continues to be allowed to create NULL columns which is all eplusr actually does and seems reasonable to continue to allow. Passes eplusr and no notice needs to be given after all. Turned back on check that j can't create NULL columns which was postponed from 1.12.2.
  • revisit changed tests and revoke where possible.
  • news items : follow up to bug fix 19 in 1.12.2, and cbind(DT, list(...)) change

@codecov
Copy link

@codecov codecov bot commented Jun 12, 2019

Codecov Report

Merging #3471 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3471      +/-   ##
==========================================
+ Coverage    98.2%   98.23%   +0.03%     
==========================================
  Files          66       66              
  Lines       12986    12926      -60     
==========================================
- Hits        12753    12698      -55     
+ Misses        233      228       -5
Impacted Files Coverage Δ
src/init.c 100% <ø> (ø) ⬆️
src/wrappers.c 100% <ø> (ø) ⬆️
R/setkey.R 98.75% <100%> (+0.4%) ⬆️
R/utils.R 100% <100%> (ø) ⬆️
R/as.data.table.R 100% <100%> (ø) ⬆️
R/data.table.R 97.87% <100%> (+0.12%) ⬆️

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 be1fae5...4b74f1f. Read the comment docs.

@mattdowle mattdowle merged commit 04eeb30 into master Jun 14, 2019
4 checks passed
@mattdowle mattdowle deleted the asdatatable branch Jun 14, 2019
sritchie73 added a commit that referenced this issue Jan 22, 2020
@sritchie73 sritchie73 mentioned this pull request Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant