add sort_by.data.table

[rikivillalba]

Proposal for #6662

• Note that sort_by.data.table() will sort using C-locale now, this is incompatible with base::sort_by.data.frame() used on data.tables

[github-actions]

Generated via commit ee44ef4

Download link for the artifact containing the test results: ↓ atime-results.zip

Task	Duration
R setup and installing dependencies	4 minutes and 34 seconds
Installing different package versions	8 minutes and 18 seconds
Running and plotting the test cases	2 minutes and 22 seconds

[MichaelChirico]

Looks good! I think it needs an entry in man/, probably in ?forder would be the most logical place?

[rikivillalba]

Looks good! I think it needs an entry in man/, probably in ?forder would be the most logical place?

Added a reference in setorder.Rd (?forder). PTAL.

[MichaelChirico]

CMIIW, we leave this as a comment instead of \method{sort_by}{data.table} because the method is not available in all supported R versions, is that right?

[rikivillalba]

Hello. I simply put another use case, these are provided as comments and printed verbatim (as comments) in manpages.
However now that you mention this, I clarified that those references to sort_by are only R>=4.4.0

[MichaelChirico]

Ah, unfortunately because of r-lib/covr#567 we won't get coverage of this code yet -- the coverage runner is stuck on an R version before sort_by() is available.

[MichaelChirico]

I am still unsure if we should try something to ensure \method{sort_by}{data.table}(x, \dots) is used "properly", but anyway LGTM, thanks!!

[MichaelChirico]

I noticed {maditr} also defines S3method(sort_by,data.table):

https://github.com/gdemin/maditr/blob/a4df8cccfe5ca5c8a919c6b6be2eb0e238c8866b/NAMESPACE#L19

cc @gdemin

I suppose that will cause breakage on release.

Anyway, there the \method{sort_by}{data.table} exists:

https://github.com/gdemin/maditr/blob/a4df8cccfe5ca5c8a919c6b6be2eb0e238c8866b/man/let_if.Rd#L27

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 98.60%. Comparing base (5a04ba8) to head (ee44ef4).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
R/data.table.R	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6679      +/-   ##
==========================================
- Coverage   98.64%   98.60%   -0.05%     
==========================================
  Files          79       79              
  Lines       14650    14656       +6     
==========================================
  Hits        14452    14452              
- Misses        198      204       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[aitap]

I've tried \method{sort_by}{data.table}(x, y, \dots) together with \alias{sort_by.data.table} and, surprisingly, it just worked on both R-4.2.2 and R-3.3.0. There's no need for scary-looking kludges like \if{\Sexpr[stage=install,results=rd]{as.character(exists("sort_by", "package:base"))}{\method{...}}. R CMD check only verifies that all exported symbols are documented, not that all documented aliases are exported (or even exist in the namespace). The old versions of R do complain about missing .formula2varlist.

[jangorecki]

This PR would benefit from example of usage in NEWS file