lintr icon indicating copy to clipboard operation
lintr copied to clipboard

Detect broken configuration

Open AshesITR opened this issue 2 years ago • 3 comments

A lot of new users seem to stumble upon the quirkiness of the dcf format we use for .lintr. I see three major pitfalls:

  1. Indentation of multiline code is not intuitive
# Wrong
linters: with_defaults(
    line_length_linter = line_length_linter(120)
)

# Right
linters: with_defaults(
    line_length_linter = line_length_linter(120)
  )
  1. Trailing newline is mandatory, but not reported as an error
# Wrong
linters: with_defaults(
    line_length_linter = line_length_linter(120)
  )
(EOF)

# Right
linters: with_defaults(
    line_length_linter = line_length_linter(120)
  )

(EOF)
  1. The proper location of the .lintr config files sometimes causes confusion.

It would be nice of us to catch these and report a warning (or maybe even an error?) if the config file is malformed.

Regarding the location issue, maybe reporting the config file used (if any) would help diagnosing the problem more quickly. I see a few options here as to when to report it:

  1. Only if some option (linter.verbose, or lintr.debug?) is set.
  2. Only if some argument (verbose, or debug?) is TRUE.
  3. Always if running interactively.
  4. Always at the start of a top-level call (lint_dir, lint_package or lint if the latter isn't called from within lint_*).

WDYT? I think the most user-friendly version would be to use getOption("lintr.???", default = interactive()) to decide whether to print this diagnostic info at the start of a top-level call. If no config file is found, the info could be

Using the default linter configuration. See vignette("using_lintr", package = "lintr") for customization options.

or something like that.

AshesITR avatar Nov 13 '21 09:11 AshesITR

@AshesITR I think this is a great idea!

stufield avatar Nov 13 '21 17:11 stufield

Even better than just detecting issues, if we can warn(), fix, and continue, that's even better. (2) in particular strikes me as something that we should be flexible on.

Is there any ambiguity that we avoid by keeping the current strict rules on the .lintr? Or is it just maintenance overhead for the .lintr parser?

(3) is a bit tougher... could we expose a show_lintr_config() helper that would give a detailed warning if no .lintr is found in the right places?

MichaelChirico avatar Nov 15 '21 01:11 MichaelChirico

(1) produces the following error in read.dcf:

Error in read.dcf(file = textConnection("linters: list(\n  1\n)")): Line starting ') ...' is malformed!

(2) appears to not cause issues in read.dcf for R 4.1.2, but I think it used to cause the same warning as readLines() in some earlier versions: https://github.com/jimhester/lintr/blob/8cd6adde3c6f08b88fcd1a280ad276784bd4f1f4/R/settings.R#L28 Can someone verify this?

(3) maybe call it lintr_sitrep(), akin to other tidyverse packages? Sounds cleaner than changing the (default) output of long-existing functions. The only downside is users may need to be pointed to it.

AshesITR avatar Nov 15 '21 16:11 AshesITR

One easy win here would be to signal that the config is malformed, e.g. I had this file:

exclusions: list(
   'tests',
 )

and lint_package() complains:

lint_package()
Error in list("tests", ) : argument 2 is empty

Not the most informative error.

MichaelChirico avatar Jun 25 '23 01:06 MichaelChirico