From 1230d5c2d33621a0741f4f8a523d48bb4bf78513 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 8 Dec 2023 15:34:22 +0000 Subject: [PATCH 1/2] give specific example of explicit types in lint message --- R/implicit_integer_linter.R | 13 ++- tests/testthat/test-implicit_integer_linter.R | 89 ++++++++++--------- 2 files changed, 56 insertions(+), 46 deletions(-) diff --git a/R/implicit_integer_linter.R b/R/implicit_integer_linter.R index 5dae749a8..ab7f2e217 100644 --- a/R/implicit_integer_linter.R +++ b/R/implicit_integer_linter.R @@ -55,12 +55,19 @@ implicit_integer_linter <- function(allow_colon = FALSE) { xml <- source_expression$full_xml_parsed_content if (is.null(xml)) return(list()) - numbers <- xml_find_all(xml, xpath) + number_expr <- xml_find_all(xml, xpath) + number <- xml_text(number_expr) + lint_idx <- is_implicit_integer(number) + number_expr <- number_expr[lint_idx] + number <- number[lint_idx] + is_negative <- !is.na(xml_find_first(number_expr, "parent::expr/preceding-sibling::OP-MINUS")) + + lint_message <- sprintf("Use %1$dL or %1$d.0 to avoid implicit integers.", ((-1) ^ is_negative) * as.integer(number)) xml_nodes_to_lints( - numbers[is_implicit_integer(xml_text(numbers))], + number_expr, source_expression = source_expression, - lint_message = "Avoid implicit integers. Use e.g. 1L for integers or 1.0 for doubles.", + lint_message = lint_message, type = "style", column_number_xpath = "number(./@col2 + 1)", # mark at end range_end_xpath = "number(./@col2 + 1)" # end after number for easy fixing (enter "L" or ".0") diff --git a/tests/testthat/test-implicit_integer_linter.R b/tests/testthat/test-implicit_integer_linter.R index 1aa57d146..9b67651bc 100644 --- a/tests/testthat/test-implicit_integer_linter.R +++ b/tests/testthat/test-implicit_integer_linter.R @@ -4,51 +4,52 @@ local({ # Note: cases indicated by "*" are whole numbers, but don't lint because the user has # effectively declared "this is a double" much as adding '.0' is otherwise accepted. cases <- tibble::tribble( - ~num_value_str, ~should_lint, - "Inf", FALSE, - "NaN", FALSE, - "TRUE", FALSE, - "FALSE", FALSE, - "NA", FALSE, - "NA_character_", FALSE, - "2.000", FALSE, - "2.", FALSE, - "2L", FALSE, - "2.0", FALSE, - "2.1", FALSE, - "2", TRUE, - "1e3", TRUE, - "1e3L", FALSE, - "1.0e3L", FALSE, - "1.2e3", FALSE, # * ( = 1200) - "1.2e-3", FALSE, - "1e-3", FALSE, - "1e-33", FALSE, - "1.2e0", FALSE, - "0x1p+0", FALSE, # * ( = 1) - "0x1.ecp+6L", FALSE, - "0x1.ecp+6", FALSE, # * ( = 123) - "0x1.ec66666666666p+6", FALSE, - "8i", FALSE, - "8.0i", FALSE + ~num_value_str, ~lint_msg, + "Inf", "", + "NaN", "", + "TRUE", "", + "FALSE", "", + "NA", "", + "NA_character_", "", + "2.000", "", + "2.", "", + "2L", "", + "2.0", "", + "2.1", "", + "2", "2L or 2.0", + "1e3", "1000L or 1000.0", + "1e3L", "", + "1.0e3L", "", + "1.2e3", "", # * ( = 1200) + "1.2e-3", "", + "1e-3", "", + "1e-33", "", + "1.2e0", "", + "0x1p+0", "", # * ( = 1) + "0x1.ecp+6L", "", + "0x1.ecp+6", "", # * ( = 123) + "0x1.ec66666666666p+6", "", + "8i", "", + "8.0i", "" ) # for convenience of coercing these to string (since tribble doesn't support auto-conversion) int_max <- .Machine[["integer.max"]] # largest number that R can represent as an integer cases_int_max <- tibble::tribble( - ~num_value_str, ~should_lint, - -int_max - 1.0, FALSE, - -int_max, TRUE, - int_max, TRUE, - int_max + 1.0, FALSE + ~num_value_str, ~lint_msg, + -int_max - 1.0, "", + -int_max, sprintf("%1$dL or %1$d.0", -int_max), + int_max, sprintf("%1$dL or %1$d.0", int_max), + int_max + 1.0, "" ) cases_int_max$num_value_str <- as.character(cases_int_max$num_value_str) cases <- rbind(cases, cases_int_max) - cases$.test_name <- sprintf("num_value_str=%s, should_lint=%s", cases$num_value_str, cases$should_lint) linter <- implicit_integer_linter() patrick::with_parameters_test_that( "single numerical constants are properly identified ", - expect_lint(num_value_str, if (should_lint) "Avoid implicit integers", linter), + { + expect_lint(num_value_str, if (nzchar(lint_msg)) lint_msg, linter) + }, .cases = cases ) }) @@ -56,7 +57,7 @@ local({ test_that("linter returns the correct linting", { linter <- implicit_integer_linter() - lint_msg <- rex::rex("Avoid implicit integers. Use e.g. 1L for integers or 1.0 for doubles.") + lint_msg <- rex::rex("Use 1L or 1.0 to avoid implicit integers.") expect_lint("x <<- 1L", NULL, linter) expect_lint("1.0/-Inf -> y", NULL, linter) @@ -67,7 +68,7 @@ test_that("linter returns the correct linting", { ) expect_lint( "z <- 1e5", - list(message = lint_msg, line_number = 1L, column_number = 9L), + list(message = rex::rex("100000L or 100000.0"), line_number = 1L, column_number = 9L), linter ) expect_lint( @@ -78,8 +79,8 @@ test_that("linter returns the correct linting", { expect_lint( "552^9", list( - list(message = lint_msg, line_number = 1L, column_number = 4L), - list(message = lint_msg, line_number = 1L, column_number = 6L) + list(message = rex::rex("552L or 552.0"), line_number = 1L, column_number = 4L), + list(message = rex::rex("9L or 9.0"), line_number = 1L, column_number = 6L) ), linter ) @@ -88,11 +89,13 @@ test_that("linter returns the correct linting", { skip_if_not_installed("tibble") patrick::with_parameters_test_that( "numbers in a:b input are optionally not linted", - expect_lint( - paste0(left, ":", right), - if (n_lints > 0L) rep(list("Avoid implicit integers"), n_lints), - implicit_integer_linter(allow_colon = allow_colon) - ), + { + expect_lint( + paste0(left, ":", right), + if (n_lints > 0L) rep(list(rex::rex("Use 1L or 1.0")), n_lints), + implicit_integer_linter(allow_colon = allow_colon) + ) + }, .cases = tibble::tribble( ~left, ~right, ~n_lints, ~allow_colon, ~.test_name, "1", "1", 2L, FALSE, "1:1, !allow_colon", From 39f4c7ae6d2c9a51660cb0abeeba612dcd18f87d Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 8 Dec 2023 15:42:02 +0000 Subject: [PATCH 2/2] delint --- R/implicit_integer_linter.R | 3 ++- tests/testthat/test-implicit_integer_linter.R | 16 ++++++---------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/R/implicit_integer_linter.R b/R/implicit_integer_linter.R index ab7f2e217..66962d4bb 100644 --- a/R/implicit_integer_linter.R +++ b/R/implicit_integer_linter.R @@ -62,7 +62,8 @@ implicit_integer_linter <- function(allow_colon = FALSE) { number <- number[lint_idx] is_negative <- !is.na(xml_find_first(number_expr, "parent::expr/preceding-sibling::OP-MINUS")) - lint_message <- sprintf("Use %1$dL or %1$d.0 to avoid implicit integers.", ((-1) ^ is_negative) * as.integer(number)) + lint_message <- + sprintf("Use %1$dL or %1$d.0 to avoid implicit integers.", ((-1L) ^ is_negative) * as.integer(number)) xml_nodes_to_lints( number_expr, diff --git a/tests/testthat/test-implicit_integer_linter.R b/tests/testthat/test-implicit_integer_linter.R index 9b67651bc..12a94a9b6 100644 --- a/tests/testthat/test-implicit_integer_linter.R +++ b/tests/testthat/test-implicit_integer_linter.R @@ -47,9 +47,7 @@ local({ linter <- implicit_integer_linter() patrick::with_parameters_test_that( "single numerical constants are properly identified ", - { - expect_lint(num_value_str, if (nzchar(lint_msg)) lint_msg, linter) - }, + expect_lint(num_value_str, if (nzchar(lint_msg)) lint_msg, linter), .cases = cases ) }) @@ -89,13 +87,11 @@ test_that("linter returns the correct linting", { skip_if_not_installed("tibble") patrick::with_parameters_test_that( "numbers in a:b input are optionally not linted", - { - expect_lint( - paste0(left, ":", right), - if (n_lints > 0L) rep(list(rex::rex("Use 1L or 1.0")), n_lints), - implicit_integer_linter(allow_colon = allow_colon) - ) - }, + expect_lint( + paste0(left, ":", right), + if (n_lints > 0L) rep(list(rex::rex("Use 1L or 1.0")), n_lints), + implicit_integer_linter(allow_colon = allow_colon) + ), .cases = tibble::tribble( ~left, ~right, ~n_lints, ~allow_colon, ~.test_name, "1", "1", 2L, FALSE, "1:1, !allow_colon",