Skip to content

key argument for as.data.table methods#3573

Merged
mattdowle merged 8 commits intomasterfrom
as_data_table_key
May 22, 2019
Merged

key argument for as.data.table methods#3573
mattdowle merged 8 commits intomasterfrom
as_data_table_key

Conversation

@MichaelChirico
Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico commented May 20, 2019

Closes #890

@jangorecki thanks for the heads up that this never ended up merged (3 years ago!)

Requesting review since there's a potentially breaking change to as.data.table.array -- key argument has to be brought to the front (S3 method consistency), so anyone that's relying on the ordering of the arguments to as.data.table.array will be affected by this PR.

PS Jan, could you please change the manual entry for the sorted argument to as.data.table.array? It gives no information about what it's used for, just that it's passed to the method.

@MichaelChirico MichaelChirico requested a review from jangorecki May 20, 2019 12:18
@codecov
Copy link
Copy Markdown

codecov bot commented May 20, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3573      +/-   ##
==========================================
+ Coverage   97.58%   97.62%   +0.03%     
==========================================
  Files          66       66              
  Lines       12695    12698       +3     
==========================================
+ Hits        12389    12397       +8     
+ Misses        306      301       -5
Impacted Files Coverage Δ
R/xts.R 100% <100%> (ø) ⬆️
R/as.data.table.R 99.15% <100%> (+4.24%) ⬆️

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 c68e95e...9bc5e27. Read the comment docs.

Comment thread R/as.data.table.R
@MichaelChirico
Copy link
Copy Markdown
Member Author

@jangorecki ok done

@mattdowle mattdowle added this to the 1.12.4 milestone May 22, 2019
@jangorecki
Copy link
Copy Markdown
Member

@MichaelChirico
Copy link
Copy Markdown
Member Author

Not quite, I missed it. Adding a test now.

@MichaelChirico
Copy link
Copy Markdown
Member Author

NVM, @jangorecki was right (though my test indeed was not working as intended), that line is un-hittable. mn > 0 case will cover naming for all normal inputs; even if mn == 0, setDT handles the naming.

@Rdatatable Rdatatable deleted a comment from codecov bot May 22, 2019
@mattdowle mattdowle changed the title Closes #890 - key argument for as.data.table methods key argument for as.data.table methods May 22, 2019
@mattdowle mattdowle merged commit 8956cee into master May 22, 2019
@mattdowle mattdowle deleted the as_data_table_key branch May 22, 2019 17:07
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.

Add an optional argument (key= ...) to as.data.table

3 participants