Skip to content

Ignore symbols used for subsetting in object_name_linter #765

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Feb 27, 2021
Merged

Ignore symbols used for subsetting in object_name_linter #765

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3d0fc88
Ignore symbols used for subsetting in `object_name_linter`
AshesITR Feb 18, 2021
c16f53b
improve XPath and add more tests
AshesITR Feb 23, 2021
22e3dea
Merge branch 'master' into fix/582-object-name-dollar-op
AshesITR Feb 23, 2021
2c9b8ac
even more thorough tests
AshesITR Feb 23, 2021
b732277
fix STR_CONST assignment lints and add tests
AshesITR Feb 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
* `object_usage_linter()` now correctly reports usage warnings spanning multiple lines (#507, @AshesITR)
* `T_and_F_symbol_linter()` no longer lints occurrences of `T` and `F` when used for subsetting and gives a better
message when used as variable names (#657, @AshesITR)
* `object_name_linter()` no longer lints names used for subsetting (#582, @AshesITR)

# lintr 2.0.1

Expand Down
19 changes: 10 additions & 9 deletions R/object_name_linters.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,15 @@ object_name_linter <- function(styles = c("snake_case", "symbols")) {
xml <- source_file$full_xml_parsed_content

xpath <- paste0(
# Left hand assignments
"//expr[SYMBOL or STR_CONST][following-sibling::LEFT_ASSIGN or following-sibling::EQ_ASSIGN]/*",

# Or
" | ",

# Right hand assignments
"//expr[SYMBOL or STR_CONST][preceding-sibling::RIGHT_ASSIGN]/*",
# assignments
"//SYMBOL[",
" not(preceding-sibling::OP-DOLLAR)",
" and ancestor::expr[",
Copy link
Collaborator

@MichaelChirico MichaelChirico Feb 19, 2021

Choose a reason for hiding this comment

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

I have a feeling ancestor may be too aggressive without more conditions. Running the comparison script now to try and confirm.

Copy link
Collaborator Author

@AshesITR AshesITR Feb 19, 2021

Choose a reason for hiding this comment

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

Sounds good. I tested "//parent::expr" before, which produced false positives (detecting symbols on the other side of assigments).
"ancestor" was the only axis that passed all the tests.

" following-sibling::LEFT_ASSIGN",
" or preceding-sibling::RIGHT_ASSIGN",
" or following-sibling::EQ_ASSIGN",
" ]",
"]",

# Or
" | ",
Expand All @@ -49,7 +50,7 @@ object_name_linter <- function(styles = c("snake_case", "symbols")) {

# Retrieve assigned name
nms <- strip_names(
xml2::xml_text(xml2::xml_find_first(assignments, "./text()"))
xml2::xml_text(assignments)
)

generics <- strip_names(c(
Expand Down
13 changes: 12 additions & 1 deletion tests/testthat/test-object_name_linter.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ test_that("styles are correctly identified", {

test_that("linter ignores some objects", {
# names for which style check is ignored
expect_lint("`%x%` <- t", NULL, object_name_linter("snake_case")) # operator
expect_lint("`%X%` <- t", NULL, object_name_linter("SNAKE_CASE")) # operator
expect_lint("`%x%` <- t", NULL, object_name_linter("snake_case")) # operator
expect_lint("`t.test` <- t", NULL, object_name_linter("UPPERCASE")) # std pkg
expect_lint(".Deprecated('x')", NULL, object_name_linter("lowercase")) # std pkg
expect_lint("print.foo <- t", NULL, object_name_linter("CamelCase")) # S3 generic
Expand Down Expand Up @@ -91,3 +91,14 @@ test_that("linter accepts vector of styles", {
linter
)
})

test_that("dollar subsetting only lints the first expression", {
# Regression test for #582
linter <- object_name_linter()
msg <- "Variable and function name style should be snake_case or symbols."

expect_lint("my_var$MY_COL <- 42L", NULL, linter)
expect_lint("MY_VAR$MY_COL <- 42L", msg, linter)
expect_lint("my_var$MY_SUB$MY_COL <- 42L", NULL, linter)
expect_lint("MY_VAR$MY_SUB$MY_COL <- 42L", msg, linter)
})