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

Add new map_vec() to purrr_mappers vector #1866

Merged
merged 4 commits into from
Dec 20, 2022
Merged

Conversation

Bisaloo
Copy link
Contributor

@Bisaloo Bisaloo commented Dec 20, 2022

No description provided.

@MichaelChirico
Copy link
Collaborator

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

IndrajeetPatil
IndrajeetPatil previously approved these changes Dec 20, 2022
Copy link
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

codecov-commenter commented Dec 20, 2022

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
Collaborator

Should map2_vec and pmap_vec also be included?

@Bisaloo
Copy link
Contributor 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
Collaborator

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
Contributor Author

Bisaloo commented Dec 20, 2022

But if I understand correctly, it clashes with

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

@MichaelChirico
Copy link
Collaborator

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
Contributor Author

Bisaloo commented Dec 20, 2022

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

@Bisaloo
Copy link
Contributor 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
Collaborator

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

Copy link
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
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
Member

@DavisVaughan DavisVaughan Dec 21, 2022

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
Collaborator

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.

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.

5 participants