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

some sapply->vapply replacements #4502

Merged
merged 3 commits into from
Jun 11, 2021
Merged

some sapply->vapply replacements #4502

merged 3 commits into from
Jun 11, 2021

Conversation

MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented May 27, 2020

Noticed while working on #4501.

Some regex:

grep -Enr "(?:any|all)\(sapply\(" R
grep -Enr "sapply\(.*is\." R

@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #4502 (953deab) into master (ad5b427) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4502   +/-   ##
=======================================
  Coverage   99.47%   99.47%           
=======================================
  Files          75       75           
  Lines       14807    14807           
=======================================
  Hits        14729    14729           
  Misses         78       78           
Impacted Files Coverage Δ
R/data.table.R 99.94% <ø> (ø)
R/as.data.table.R 100.00% <100.00%> (ø)
R/cedta.R 100.00% <100.00%> (ø)
R/fread.R 100.00% <100.00%> (ø)
R/groupingsets.R 100.00% <100.00%> (ø)
R/setkey.R 100.00% <100.00%> (ø)
R/utils.R 100.00% <100.00%> (ø)

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 ad5b427...953deab. Read the comment docs.

@jangorecki
Copy link
Member

jangorecki commented May 27, 2020

Personaly I don't like our internal vapply_* family.
I would prefer to have something like this, just to re-order arguments

vapply2 = function(FUN.VALUE, ...) vapply(FUN.VALUE=FUN.VALUE, ...)

and use it

vapply2(NA, x, is.numeric)
vapply2(1L, y, length)
vapply2("", z, paste, collapse="")

or just vapply is good enough

@mattdowle mattdowle added this to the 1.14.1 milestone Jun 11, 2021
@mattdowle
Copy link
Member

On @jangorecki's comment the only minor thoughts I have on vapply2 is that I generally don't remember what the version 2 is for on such versioned function names and prefer instead a letter or word or different name .. like vapply_1b I remember 1b means length-1 boolean, and sapply I remember the s stands for simplify. Also the *apply* family taking the first argument as what to apply to is pretty strong in my mind.

@mattdowle mattdowle merged commit 36e1b02 into master Jun 11, 2021
@mattdowle mattdowle deleted the sapply-vapply branch June 11, 2021 18:28
@jangorecki jangorecki modified the milestones: 1.14.9, 1.15.0 Oct 29, 2023
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.

3 participants