lint(text=) finds local settings again

[MichaelChirico]

Closes #2847, cc @bastistician

Not the biggest fan of the new parse_settings=FALSE usages, open to ideas about what needs to be improved here...

PS, I originally pushed this directly to main by mistake as b548c78; reverted as dc7a612 to put this through normal review

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.26%. Comparing base (afb363c) to head (1ba8f42).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2863   +/-   ##
=======================================
  Coverage   99.26%   99.26%           
=======================================
  Files         127      127           
  Lines        7092     7092           
=======================================
  Hits         7040     7040           
  Misses         52       52           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[MichaelChirico]

Hm, I think this is having an interaction with the lack of {cyclocomp}. IINM package examples of lint(text=.) are trying to use our ./.lintr config, which requires having {cyclocomp}. Testing if 159234d works.

[AshesITR]

I wonder if there is a better way to allow requesting the old behavior.

Looking at the diff, the blast radius seems quite large.

[AshesITR]

This we should avoid I think. It could inadvertently disable the linter for its core purpose (linting lintr) if the dev dependencies aren't correct elsewhere.

[MichaelChirico]

True... any ideas? The problem is that both R-CMD-check-hard and R-CMD-check GHA use this same config, but the former can't work without cyclocomp_linter().

[AshesITR]

I think it was a good idea to not parse settings by default for lint(text=).
If we want to go back on that, maybe add a step in R-CMD-check-hard that removes our .lintr file entirely before running?

[MichaelChirico]

I thought so too, however, I don't know how else to restore the Emacs behavior described in the issue.

[AshesITR]

If I understand the (7 year old) ess-lint code correctly, it gives all arguments to lint() by position.
We could soft-deprecate parsing settings if the code is pushed in via file= instead of text=.
WDYT?

[MichaelChirico]

What's the deprecation path, exactly? Would it suffice for tools like ess-lint to explicitly supply parse_settings=TRUE? Or such tools are required to do full-file parsing instead?

[AshesITR]

I think explicitly requesting parse_settings = TRUE is a good way forward.

[bastistician]

Two comments from the outside:

1. The use of parse_settings=FALSE in tests doesn't seem to be a bad idea, citing from vignette("lintr") (emphasis mine):

   If only the specified settings should be changed, and the remaining settings should be taken directly from the defaults, the argument parse_settings = FALSE can be added to the function calls. This will suppress reading of the .lintr configuration. This is particularly useful for tests which should not exclude example files containing lints while the package-level .lintr excludes those files because the lints are intentional.

2. For the purpose of ess-r-flymake, it seems to be sufficient to keep respecting options("lintr.linter_file") when that gives an absolute path, even when linting "inline" code. Maybe this can simplify things and avoid the proposed read_settings() kludge filename <- "./any_local_file"?