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

Additional test for unnecessary_placeholder_linter() #1838

Merged
merged 4 commits into from
Dec 20, 2022

Conversation

IndrajeetPatil
Copy link
Collaborator

@IndrajeetPatil IndrajeetPatil commented Dec 12, 2022

To make sure that we don't lint valid usage of placeholder here:

library(magrittr)
1:4 %>% { sum(.) }
#> [1] 10
x <- 1:4
x %<>% { sum(.) }
x
#> [1] 10

Created on 2022-12-12 with reprex v2.0.2

Without the placeholder, the results are incorrect:

library(magrittr)
1:4 %>% { sum() }
#> [1] 0

x <- 1:4
x %<>% { sum() }
x
#> [1] 0

Created on 2022-12-12 with reprex v2.0.2

@MichaelChirico
Copy link
Collaborator

sorry, a bit confused at the PR description vs actual update. is the outcome of this not the same?

1:4 %>% sum()
x = 1:4
x %<>% sum()

@IndrajeetPatil
Copy link
Collaborator Author

Sorry for the confusion:

The examples in the top-level comment were just to show how a placeholder (.) is needed here and that it'd be good to have a test that ensures that we are not producing a false positive in such contexts (we aren't).

It's important that there not be a false positive here because the lint message recommends removing ., and that would produce an incorrect result here:

library(magrittr)
1:4 %>% { sum() }
#> [1] 0

x <- 1:4
x %<>% { sum() }
x
#> [1] 0

Created on 2022-12-12 with reprex v2.0.2

@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2022

Codecov Report

Merging #1838 (67860f4) into main (7dfe5d6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1838   +/-   ##
=======================================
  Coverage   98.86%   98.86%           
=======================================
  Files         112      112           
  Lines        4839     4839           
=======================================
  Hits         4784     4784           
  Misses         55       55           

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

@IndrajeetPatil IndrajeetPatil added this to the 3.1.0 milestone Dec 19, 2022
@MichaelChirico MichaelChirico merged commit 984c18f into main Dec 20, 2022
@MichaelChirico MichaelChirico deleted the scope_test_placeholder_linter branch December 20, 2022 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants