Absolute paths detection is broken

[etiennebacher]

flint::lint_text("paste('/')")
#> Original code: / 
#> Suggestion: Do not use absolute paths. 
#> Rule ID: absolute_path-1

[KlausVigo]

Hi @etiennebacher,

nice and useful package, especially for a . like me. The help pages are often still very sparse, I personally like as many examples as possible.

Here are a few more false positives I detected of absolute_path when I tried flint_package it on phangorn. In fact this was the most common linter (183) I experienced.

cat("\n")
cat("\t")
cat("\"")

Additionally absolute_path is not listed in list_linters(). Is list_linters() hard coded as a vector?

I noticed most id's end with "-1", "-2", but some with "_1" (true_false_symbol, length_test).

Kind regards,
Klaus

PS:
I know the linter rules are a bit opinionated. I would most of the time prefer expect_equal over expect_identical, especially as many objects might contain floating point numbers with possible numerical errors.

> fun <- function(x) sqrt(x)^2
> all.equal(9, fun(9))
[1] TRUE
> identical(9, fun(9))
[1] TRUE
> all.equal(10, fun(10))
[1] TRUE
> identical(10, fun(10))
[1] FALSE

Maybe a linter for detecting == or identical when comparing floating point numbers with integers (and floating point numbers with each other)? This can be really hard to catch errors.

[etiennebacher]

Hi @KlausVigo, thanks for trying flint and for the detailed feedback!

The help pages are often still very sparse, I personally like as many examples as possible.

That's true, the reason is that there are many linters and almost all come from the lintr list. Therefore, I didn't want to spend too much time on this and instead automated the doc generation to contain a link to the lintr documentation. I agree this is not ideal for the end user. This will take some time but I think it's a good way to get started to contributing to flint if you're interested.

Additionally absolute_path is not listed in list_linters(). Is list_linters() hard coded as a vector?

Yes it is, it's here: https://github.com/etiennebacher/flint/blob/02ae2d21d9833742878c609e7144102d0101e68e/R/list-linters.R#L8
absolute_path_linter had way too many false positives that I couldn't solve quickly so I chose to deactivate it in 0.0.5 (released end of August 2024). So I'm actually surprised you ran into issues with it since it shouldn't be run by default, did you add it to the list of linters yourself?

I noticed most id's end with "-1", "-2", but some with "_1" (true_false_symbol, length_test).

Thanks, I'll fix that.

I know the linter rules are a bit opinionated. I would most of the time prefer expect_equal over expect_identical, especially as many objects might contain floating point numbers with possible numerical errors.

It's true, but that's also why you can use the argument exclude_linters:

flint::lint_text("expect_equal(10, sqrt(10)^2)")
#> Original code: expect_equal(10, sqrt(10)^2) 
#> Suggestion: Use expect_identical(x, y) by default; resort to expect_equal() only when
#> needed, e.g. when setting ignore_attr= or tolerance=. 
#> Rule ID: expect_identical-2

flint::lint_text(
  "expect_equal(10, sqrt(10)^2)",
  exclude_linters = "expect_identical"
)

(and you can use setup_flint() and customize the config file to avoid adding this argument every time.)

The difficulty here is that I can't run R code in the linter rules, I can only rely on code pattern. For instance, if you pass an object x in expect_equal(), I can't know whether this is a double, an integer, or something else.

[adamhsparks]

I've been getting the false positives with absolute_path_linter, and note that I didn't add it manually. I just used setup_flint() with the recent version from here.

For me, it was picking up "/n" as well, which I "fixed" by using cli::cat_line() since I was already using {cli}. But it's also detecting regexes, e.g., precompiling the vignette uses this, abares_replace <- gsub("\\(figure/", "\\(", abares_replace) and it's picking up the \\ and flagging it.

I'm fine to ignore these false flags. The package has been invaluable otherwise picking things up that others had missed!

[etiennebacher]

Thanks for the report, this was indeed a mistake on my side. I had deactivated this linter in 0.0.5 but it came back when I refactored some code. Could you get the current github version and confirm that this linter is now ignored?

[adamhsparks]

Can confirm the latest version does not pick up the abares_replace <- gsub("\\(figure/", "\\(", abares_replace)