Skip to content

Writing R code in a DCF is unappealing #1210

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hadley opened this issue May 24, 2022 · 21 comments · Fixed by #2177
Closed

Writing R code in a DCF is unappealing #1210

hadley opened this issue May 24, 2022 · 21 comments · Fixed by #2177
Labels
breaking change ☠️ API change likely to affect existing code config
Milestone

Comments

@hadley
Copy link
Member

hadley commented May 24, 2022

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?

@AshesITR
Copy link
Collaborator

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
Copy link
Collaborator

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?

@hadley
Copy link
Member Author

hadley commented May 24, 2022

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.

@AshesITR
Copy link
Collaborator

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.

@hadley
Copy link
Member Author

hadley commented May 24, 2022

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.

@AshesITR
Copy link
Collaborator

AshesITR commented May 24, 2022

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

@MichaelChirico
Copy link
Collaborator

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

@AshesITR
Copy link
Collaborator

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

@MichaelChirico
Copy link
Collaborator

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

@AshesITR
Copy link
Collaborator

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.

@MichaelChirico
Copy link
Collaborator

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

@AshesITR
Copy link
Collaborator

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

@hadley
Copy link
Member Author

hadley commented May 25, 2022

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

@MichaelChirico
Copy link
Collaborator

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

@MichaelChirico
Copy link
Collaborator

I guess this would supersede #886

@eitsupi
Copy link
Contributor

eitsupi commented May 25, 2022

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

@AshesITR
Copy link
Collaborator

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.

@HaHeho
Copy link

HaHeho commented Jan 29, 2024

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.

@MichaelChirico
Copy link
Collaborator

@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.

@HaHeho
Copy link

HaHeho commented Jan 29, 2024

@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?

@MichaelChirico
Copy link
Collaborator

Should I file a bug report for this one?

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

config <- read.dcf(config_file, all = TRUE)

Please file a new bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ☠️ API change likely to affect existing code config
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants