Skip to content

Add new map_vec() to purrr_mappers vector#1866

Merged
MichaelChirico merged 4 commits intor-lib:mainfrom
Bisaloo:map_vec
Dec 20, 2022
Merged

Add new map_vec() to purrr_mappers vector#1866
MichaelChirico merged 4 commits intor-lib:mainfrom
Bisaloo:map_vec

Conversation

@Bisaloo
Copy link
Copy Markdown
Collaborator

@Bisaloo Bisaloo commented Dec 20, 2022

No description provided.

@MichaelChirico
Copy link
Copy Markdown
Collaborator

MichaelChirico commented Dec 20, 2022

thanks! could you add yourself to the NEWS item and add a test for this?

IndrajeetPatil
IndrajeetPatil previously approved these changes Dec 20, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@IndrajeetPatil IndrajeetPatil left a comment

Choose a reason for hiding this comment

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

Oh, purrr 1.0.0 is already on CRAN. Cool!

Thanks.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 20, 2022 •
edited
Loading

Codecov Report

Merging #1866 (b3d6a0e) into main (67576ef) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1866   +/-   ##
=======================================
  Coverage   98.86%   98.86%           
=======================================
  Files         112      112           
  Lines        4839     4839           
=======================================
  Hits         4784     4784           
  Misses         55       55
Impacted Files Coverage Δ
R/unnecessary_lambda_linter.R 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@MichaelChirico
Copy link
Copy Markdown
Collaborator

MichaelChirico commented Dec 20, 2022

Should map2_vec and pmap_vec also be included?

@Bisaloo
Copy link
Copy Markdown
Collaborator Author

Bisaloo commented Dec 20, 2022

Should map2_vec and pmap_vec also be included?

Then all map2() and pmap() variants should be added, which is currently not the case. But since mapply() is covered, I guess it makes sense to put them as well.

@MichaelChirico
Copy link
Copy Markdown
Collaborator

MichaelChirico commented Dec 20, 2022

ah, nvm, I didn't read the docs enough... actually I'm surprised mapply is there, maybe it shouldn't be. the logic would be different for the >1-arg function case

@Bisaloo
Copy link
Copy Markdown
Collaborator Author

Bisaloo commented Dec 20, 2022

But if I understand correctly, it clashes with

lintr/R/unnecessary_lambda_linter.R

Line 34 in 7657984

# a. it's a one-variable function [TODO(michaelchirico): is this necessary?]

@MichaelChirico
Copy link
Copy Markdown
Collaborator

MichaelChirico commented Dec 20, 2022

I guess I'd you're using mapply(function(x) x, 1:10) that is indeed a lint but I still assume the logic is off more generally. anyway, that's for a follow-up issue

@Bisaloo
Copy link
Copy Markdown
Collaborator Author

Bisaloo commented Dec 20, 2022

On the same note, outer() behaves differently than the others as well

@Bisaloo
Copy link
Copy Markdown
Collaborator Author

Bisaloo commented Dec 20, 2022

thanks! could you add yourself to the NEWS item and add a test for this?

Should I just expand on the existing news bullet rather than adding a new one since there hasn't been a release since this linter was created?

lintr/NEWS.md

Lines 98 to 100 in 67576ef

* `unnecessary_lambda_linter()`: detect unnecessary lambdas (anonymous functions), e.g.
`lapply(x, function(xi) sum(xi))` can be `lapply(x, sum)` and `purrr::map(x, ~quantile(.x, 0.75, na.rm = TRUE))`
can be `purrr::map(x, quantile, 0.75, na.rm = TRUE)`. Naming `probs = 0.75` can further improve readability (#1531, @MichaelChirico).

@MichaelChirico
Copy link
Copy Markdown
Collaborator

MichaelChirico commented Dec 20, 2022

yes exactly, I would just add to the bullet with your name and this PR number

MichaelChirico
MichaelChirico approved these changes Dec 20, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@MichaelChirico MichaelChirico left a comment

Choose a reason for hiding this comment

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

thanks!

@MichaelChirico MichaelChirico merged commit 2c5ad6d into r-lib:main Dec 20, 2022
@Bisaloo Bisaloo deleted the map_vec branch December 21, 2022 08:16
DavisVaughan
DavisVaughan reviewed Dec 21, 2022
View reviewed changes
Comment thread tests/testthat/test-unnecessary_lambda_linter.R
Comment on lines +84 to +86
"purrr::map_vec(x, ~foo(.x, y))",
rex::rex("Pass foo directly as a symbol to map_vec()"),
unnecessary_lambda_linter()
Copy link
Copy Markdown
Member

@DavisVaughan DavisVaughan Dec 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@MichaelChirico is this recommending you use map_vec(x, foo, y = y) instead?

We actually recommend using the anonymous function form rather than using ... to pass named arguments through to foo() now. Using the anonymous function form makes it a little easier to see which arguments belong to which function (i.e. does y belong to map_vec() or foo()?), and tends to give slightly better error messages. See the new documentation of ... at https://purrr.tidyverse.org/reference/map.html for more.

Copy link
Copy Markdown
Collaborator

@MichaelChirico MichaelChirico Dec 21, 2022

Choose a reason for hiding this comment

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

Thanks Davis! Given that recommendation I think we should exclude purrr mappers by default; filed #1871 to handle that before release.

@MichaelChirico MichaelChirico mentioned this pull request Dec 21, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaelChirico MichaelChirico MichaelChirico approved these changes

@DavisVaughan DavisVaughan DavisVaughan left review comments

@IndrajeetPatil IndrajeetPatil IndrajeetPatil left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Bisaloo @MichaelChirico @codecov-commenter @IndrajeetPatil @DavisVaughan