extend assignment_linter for other operators #928

MichaelChirico · 2022-03-13T01:28:23Z

Closes #915

…signment-right

MichaelChirico · 2022-03-13T06:38:37Z

@AshesITR PTAL at the test failure. Looks odd to me. For now I just removed the test.

At HEAD, assignment_linter() generates a lint with ranges=NULL:

dput(lintr::lint("a=1\n", lintr::assignment_linter()))
# structure(list(structure(list(filename = "<text>", line_number = 1L, 
#     column_number = 2L, type = "style", message = "Use <-, not =, for assignment.", 
#     line = c(`1` = "a=1"), ranges = NULL, linter = "lintr"), class = "lint")), class = "lints")

vs. in this PR it's ranges = list(c(2L, 2L))

dput(lintr::lint("a=1\n", lintr::assignment_linter()))
# structure(list(structure(list(filename = "<text>", line_number = 1L, 
#     column_number = 2L, type = "style", message = "Use <-, not =, for assignment.", 
#     line = c(`1` = "a=1"), ranges = list(c(2L, 2L)), linter = "lintr"), class = "lint")), class = "lints")

What's puzzling is the test is checking that exactly the new value of ranges isn't produced by assignment_linter():

lintr/tests/testthat/test-expect_lint.R

Line 28 in 6f55ef5

expect_failure(expect_lint("a=1", list(ranges = list(c(2L, 2L))), linter))

So I'm second guessing if there's a reason for that? I can't think of any. And ranges will do well for printing the lint of -> (and <<- and ->>):

lintr::lint("a->1\n", lintr::assignment_linter())
# <text>:1:2: style: Use <-, not ->, for assignment.
# a->1
#  ^~

There's this comment in the original PR:

I found out that it was practically impossible to check the "ranges" of a lint using expect_lint().

tests/testthat/test-assignment_linter.R

AshesITR · 2022-03-13T07:33:31Z

tests/testthat/test-expect_lint.R

-  expect_failure(expect_lint("a=1", list(ranges = list(c(2L, 2L))), linter))
+  expect_success(expect_lint("a=1", list(ranges = list(c(2L, 2L))), linter))


What was the original failure?

Error: check #1: ranges NULL did not match list(c(2L, 2L))

So that seems like a way of documenting the bug we just fixed 😆
Feel free to remove the test.

Despite not really knowing the purpose of the test I'm still a bit loath to remove it 😆

I changed it to use a different linter that gives NULL for ranges

R/assignment_linter.R

MichaelChirico · 2022-03-13T08:03:34Z

ah poop. git is hard.

R/assignment_linter.R

…ight

…signment-right

AshesITR · 2022-03-13T18:47:46Z

R/assignment_linter.R

+      # always block = (NB: the parser differentiates EQ_ASSIGN, EQ_SUB, and EQ_FORMALS)
+      "//EQ_ASSIGN",
+      # -> and ->> are both 'RIGHT_ASSIGN'
+      if (!allow_right_assign) "//RIGHT_ASSIGN" else if (!allow_cascading_assign) "//RIGHT_ASSIGN[text() = '->>']",
      # <-, :=, and <<- are all 'LEFT_ASSIGN'; check the text if blocking <<-


Shouldn't we lint := as well?

We'd have to write special logic to handle correct rlang and data.table usages. Don't think it's worth it (most people don't know that := can be used for "regular" assignment and it's explicitly not mentioned in the R documentation).

Agreed. Maybe worth mentioning here why we chose not to lint :=?

AshesITR · 2022-03-13T18:51:01Z

Looks mostly good, just a few finishing touches needed.
Nice work!

AshesITR · 2022-03-13T20:53:10Z

NEWS.md

@@ -88,6 +88,7 @@ function calls. (#850, #851, @renkun-ken)
 * `lintr` now uses the 3rd edition of `testthat` (@MichaelChirico, #910)
 * `lintr` is adopting a new set of linters provided as part of Google's extension to the tidyverse style guide (#884, @michaelchirico)
   + `expect_null_linter()` Require usage of `expect_null(x)` over `expect_equal(x, NULL)` and similar
+* `assignment_linter()` now blocks right assignment (`->` and `->>`) and gains two arguments. `allow_cascading_assign` (`TRUE` by default) toggles whether to block `<<-` and `->>`; `allow_right_assign` toggles whether to block `->` and `->>` (#915, @michaelchirico)


block --> lint

AshesITR · 2022-03-13T20:54:27Z

R/assignment_linter.R

+      # always block = (NB: the parser differentiates EQ_ASSIGN, EQ_SUB, and EQ_FORMALS)
+      "//EQ_ASSIGN",
+      # -> and ->> are both 'RIGHT_ASSIGN'
+      if (!allow_right_assign) "//RIGHT_ASSIGN" else if (!allow_cascading_assign) "//RIGHT_ASSIGN[text() = '->>']",
      # <-, :=, and <<- are all 'LEFT_ASSIGN'; check the text if blocking <<-


Agreed. Maybe worth mentioning here why we chose not to lint :=?

MichaelChirico and others added 4 commits March 13, 2022 01:27

extend assignment_linter for other operators

2485cbb

Merge branch 'master' into assignment-right

beed246

expect_success not failure

044b6e7

Merge branch 'assignment-right' of github.com:jimhester/lintr into as…

a647042

…signment-right

AshesITR reviewed Mar 13, 2022

View reviewed changes

MichaelChirico force-pushed the assignment-right branch from c774fd2 to a647042 Compare March 13, 2022 08:05

Merge branch 'master' into assignment-right

e9b8401

AshesITR reviewed Mar 13, 2022

View reviewed changes

R/assignment_linter.R Outdated Show resolved Hide resolved

MichaelChirico added 4 commits March 13, 2022 09:54

Merge branch 'master' of github.com:jimhester/lintr into assignment-r…

0cff86e

…ight

Merge branch 'assignment-right' of github.com:jimhester/lintr into as…

5d5ecd0

…signment-right

use cleaner xpath and switch to two arguments

dc3432d

roxygenize

e4b5d42

AshesITR reviewed Mar 13, 2022

View reviewed changes

change to a different linter with NULL ranges

7b62ada

AshesITR reviewed Mar 13, 2022

View reviewed changes

fine-tune comments, NEWS

abfb041

AshesITR approved these changes Mar 13, 2022

View reviewed changes

MichaelChirico merged commit 53b5f51 into master Mar 13, 2022

MichaelChirico deleted the assignment-right branch March 13, 2022 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

extend assignment_linter for other operators #928

extend assignment_linter for other operators #928

Uh oh!

MichaelChirico commented Mar 13, 2022

Uh oh!

MichaelChirico commented Mar 13, 2022

Uh oh!

Uh oh!

AshesITR Mar 13, 2022

Uh oh!

MichaelChirico Mar 13, 2022

Uh oh!

AshesITR Mar 13, 2022

Uh oh!

MichaelChirico Mar 13, 2022

Uh oh!

Uh oh!

Uh oh!

MichaelChirico commented Mar 13, 2022

Uh oh!

Uh oh!

AshesITR Mar 13, 2022

Uh oh!

MichaelChirico Mar 13, 2022

Uh oh!

AshesITR Mar 13, 2022

Uh oh!

AshesITR commented Mar 13, 2022

Uh oh!

AshesITR Mar 13, 2022

Uh oh!

AshesITR Mar 13, 2022

Uh oh!

Uh oh!

		expect_failure(expect_lint("a=1", list(ranges = list(c(2L, 2L))), linter))
		expect_success(expect_lint("a=1", list(ranges = list(c(2L, 2L))), linter))

extend assignment_linter for other operators #928

extend assignment_linter for other operators #928

Uh oh!

Conversation

MichaelChirico commented Mar 13, 2022

Uh oh!

MichaelChirico commented Mar 13, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MichaelChirico commented Mar 13, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AshesITR commented Mar 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!