lintr icon indicating copy to clipboard operation
lintr copied to clipboard

Writing R code in a DCF is unappealing

Open hadley opened this issue 3 years ago • 17 comments

Since you don't get syntax highlighting, help, autocomplete, ...

What do you think about supporting some additional way of defining custom linters that uses a file with a .R extension?

hadley avatar May 24 '22 16:05 hadley

Not sure about the .R extension. Most IDEs allow you to configure the default language per extension, and that makes it easy to confuse the config with an actual (productive) script within the project.

Other packages tend to use yaml for configs, PyCharms language injection allows for R syntax highlighting and autocompletion to be used. Plus, YAML is quite close to DCF, to the point that interpreting our .lintr as a YAML file works too:

image

AshesITR avatar May 24 '22 18:05 AshesITR

Rather minimal changes to the .lintr file would make it parseable R code, though. I'd be okay with allowing .lintr to also (in addition to being dcf-formatted) be parseable R code, restricted to assigning the lintr config variables, i.e., for our .lintr

linters <- linters_with_defaults( # The following TODOs are part of an effort to have {lintr} lint-free (#584)
   line_length_linter = line_length_linter(120),
   # TODO add implicit_integer_linter()
   cyclocomp_linter = cyclocomp_linter(37), # TODO reduce to 15
)
exclusions <- list(
  "inst/doc/creating_linters.R" = 1,
  "inst/example/bad.R",
  "tests/testthat/default_linter_testcode.R",
  "tests/testthat/dummy_packages",
  "tests/testthat/dummy_projects",
  "tests/testthat/exclusions-test",
  "tests/testthat/knitr_formats",
  "tests/testthat/knitr_malformed"
)

@MichaelChirico WDYT?

AshesITR avatar May 24 '22 18:05 AshesITR

Allowing .lintr to be either DCF or R doesn't seem that appealing to me either. I was thinking more of something like _lintr.R

There's some related art in Roxygen2 — you can configure simple options in DESCRIPTION or provide man/roxygen/meta.R if you want to run R code.

hadley avatar May 24 '22 18:05 hadley

Not a big fan of allowing two separate configuration files - what to do if both files exist in the same directory? pkgdown, blogdown, covr and GitHub Actions use YAML for configuration, by the way. I've never heard of the man/roxygen/meta.R feature.

I must admit though, that the DCF format took a bit to get used to, especially the akward indentation requirements.

AshesITR avatar May 24 '22 19:05 AshesITR

The easiest solution is to error if both files are present.

If you find the idea of a .R file unpalatable, I think yaml would also work, but you'd need to put some thought into how to parameterise it so that fewer configs would need to include executable R code.

hadley avatar May 24 '22 19:05 hadley

Hmm that seems hard because we intend on linters being configured via linters_with_defaults() or linters_with_tags(). For a transitional period, I'd be fine if .lintr would be parsed as YAML and, if that fails, as DCF with a deprecation warning. I guess the question is, can we avoid R code by leveraging the YAML structure, e.g. using

linters:
 - with: defaults
   line_length_linter:
     length: 120
   cyclocomp_linter:
     limit: 37
exclusions:
 - inst/doc/creating_linters.R: 1
 - inst/example/bad.R
 - tests/testthat/default_linter_testcode.R
 - tests/testthat/dummy_packages
 - tests/testthat/dummy_projects
 - tests/testthat/exclusions-test
 - tests/testthat/knitr_formats
 - tests/testthat/knitr_malformed

AshesITR avatar May 24 '22 21:05 AshesITR

I also find the DCF approach a bit awkward... what's the appeal of it besides back-compatibility?

MichaelChirico avatar May 25 '22 06:05 MichaelChirico

The only other advantage I can think of is read.dcf() is in base R.

AshesITR avatar May 25 '22 06:05 AshesITR

the R-based approach just needs source() though? maybe I'm not understanding how the R approach is intended to work, exactly

MichaelChirico avatar May 25 '22 06:05 MichaelChirico

Yeah, right. If we want to use R as the "format". I was comparing to YAML. We could do something like evalq(source(".lintr", encoding = ??, local = TRUE), envir = new.env(parent = ??)) for R. But we should definitely check for non-expected settings generated in the env.

AshesITR avatar May 25 '22 06:05 AshesITR

right... that basically maps to the current setup though right? DCF keys/values --> variable names/values in config environment

MichaelChirico avatar May 25 '22 06:05 MichaelChirico

True. But in R code it's more tempting to define other variables.

AshesITR avatar May 25 '22 07:05 AshesITR

If you were worried about that you could inspect the environment and error if there were unexpected variables.

PS. I'd suggest sys.source(".lintr", env = new.env(baseenv())) instead of using source() PPS. I'd also suggest using _lintr.R or similar instead of reusing the existing file name

hadley avatar May 25 '22 13:05 hadley

if we're going to switch we could also offer a script (maybe a snippet in NEWS) to convert an existing DCF->R

MichaelChirico avatar May 25 '22 14:05 MichaelChirico

I guess this would supersede #886

MichaelChirico avatar May 25 '22 14:05 MichaelChirico

Isn't it a more common method to validate YAML with a schema?

eitsupi avatar May 25 '22 14:05 eitsupi

If we go with R, we should export a lintr_config() or lintr_settings() function that has all configuration options as arguments and does some validation.

AshesITR avatar May 26 '22 21:05 AshesITR

I'm just using lintr for the first time. It's a great tool, but oh boy, is DCF terrible (since it is very easy to break).

Finding an alternative way for configuration is very desirable. The .lintr.R way might be fine. However, in the current state, this seems to suffer from similar problems due to the underlying DCF framework. E.g., impractical "malformatting" by not having the closing brackets indented (see here) still occurs with the R syntax. This formatting requirement should be removed (for good) since it does not exist in R itself.

HaHeho avatar Jan 29 '24 14:01 HaHeho

@HaHeho Could you share an R file that's giving you this issue (in a new bug report please)?

.lintr.R is plain R code, completely independent of DCF implementation, so there should be no such issue.

MichaelChirico avatar Jan 29 '24 14:01 MichaelChirico

@HaHeho Could you share an R file that's giving you this issue (in a new bug report please)?

.lintr.R is plain R code, completely independent of DCF implementation, so there should be no such issue.

Hmm, maybe my R Studio session was borked then. I started with .lintr DCF and ran some linting. I then found the release notes on the experimental feature, renamed my file to .lintr.R, and adjusted the syntax. The linter recognized the file. However, it still complained about an invalid DCF format due to the indentation.

I did some more testing now by repeating the above process. I learned that executing lintr::lint("file") in the console works fine, but executing the adding via a keyboard shortcut causes the following error.

> lintr:::addin_lint()
Error in read.dcf(config_file, all = TRUE) : Invalid DCF format.
Regular lines must have a tag.
Offending lines start with:
  linters <- linters_with_defaults(
  )

The .lintr.R file is an almost empty:

linters <- linters_with_defaults(
  object_name_linter = NULL
)

Is this a known issue that can be fixed easily? Should I file a bug report for this one?

HaHeho avatar Jan 29 '24 14:01 HaHeho

Should I file a bug report for this one?

Indeed it looks like a bug, thanks! This read.dcf() usage needs to be updated:

https://github.com/r-lib/lintr/blob/106dfb4036a63d8bbe211b0989ad09723fd0f312/R/addins.R#L15

Please file a new bug.

MichaelChirico avatar Mar 12 '24 06:03 MichaelChirico