Skip to content

Conversation

etiennebacher
Copy link
Contributor

Fixes #2737

I know this is a big diff to review, I don't mind closing this if you'd rather do that piece by piece as is the current approach.

Notes:

  • this PR would close Clean-up: use expect_no_lint() where appropriate #2737 but the issue mentions adding a linter to detect future cases of expect_lint(..., NULL, ...), so maybe you don't want to close the issue if this ends up being merged;
  • for some cases, the replacement spans several lines. I applied styler on those files but nothing changed, so I suppose the current formatting is okay for you.

Context: I did a similar replacement in flir a few weeks ago, and it went very quickly by using flir itself. Just for information (and a bit of self-promotion ;) ), here are the steps I followed:

  • flir::setup_flir(".")
  • flir::add_new_rule("expect_no_lint", ".")
  • set up the rule:
id: expect_no_lint
language: r
severity: warning
rule:
  pattern: expect_lint($A, NULL, $B)
fix: expect_no_lint(~~A~~, ~~B~~)
message: foo
  • flir::fix_dir("tests", linters = "expect_no_lint")
  • clean up a couple of cases because I had accidentally fixed other R files in the tests folder, e.g. default_linter_testcode.R

@MichaelChirico
Copy link
Collaborator

Thanks Etienne, as mentioned for #2950, I'd prefer to avoid any conflicts. It would be appreciated if you could drop any files here that intersect with other pending PRs.

Copy link

codecov bot commented Oct 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.24%. Comparing base (27a959d) to head (f7c1294).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2951   +/-   ##
=======================================
  Coverage   99.24%   99.24%           
=======================================
  Files         129      129           
  Lines        7277     7277           
=======================================
  Hits         7222     7222           
  Misses         55       55           

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

@etiennebacher
Copy link
Contributor Author

etiennebacher commented Oct 17, 2025

According to your script in the linked PR, there are 91 R files in "tests" that are currently modified by other PRs. I would argue that it's up to each PR to fix merge conflicts when there are any, especially since most of them are not actively worked on? (I may be wrong, this is all based on quick look)

Sorry, but I don't plan to go through all of those to avoid potential conflicts. But again, I totally understand if you'd rather not merge this one as-is.

@Bisaloo
Copy link
Collaborator

Bisaloo commented Oct 18, 2025

I'd be wary of conflicts if they affected new contributors but it seems almost all currently open PRs come from lintr authors, or previous contributors.

Speaking from myself, I'd be fine with resolving conflicts in my PRs, especially if it means we resolve #2737 once and for all and avoid unrelated changes when reviewing PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clean-up: use expect_no_lint() where appropriate

3 participants