Avoid implicit assignments in expectations

[IndrajeetPatil]

Closes #1820

[codecov-commenter]

Codecov Report

Merging #1879 (bfae8af) into main (4f12a43) will not change coverage.
The diff coverage is n/a.

❗ Current head bfae8af differs from pull request most recent head 951b913. Consider uploading reports for the commit 951b913 to get more accurate results

@@           Coverage Diff           @@
##             main    #1879   +/-   ##
=======================================
  Coverage   98.84%   98.84%           
=======================================
  Files         112      112           
  Lines        4857     4857           
=======================================
  Hits         4801     4801           
  Misses         56       56           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[IndrajeetPatil]

@AshesITR Before I continue with this, just wanted to double-check with you if this is what you had in mind for fixing #1820.

[IndrajeetPatil]

I haven't touched the tests for deprecated functions, which also contains some implicit assignment lints.

TBH, I think we should nolint all test files for deprecated functions; we are going to remove them at some point anyway.

[IndrajeetPatil]

The new warning appearing in tests is unrelated to the current PR. I have created an issue to track this: #1905.

[IndrajeetPatil]

@AshesITR This is ready for a review, btw. Tagging just in case you missed the original review request during the holidays.

[AshesITR]

Looks good. Just wondering if we should commit to the brace-syntax from magrittr or grab the result in an extra supressWarnings() for the xml_nodes_to_lints() tests.

The former would not be compatible with the native pipe once we drop support for R versions without it.

[IndrajeetPatil]

Looks good. Just wondering if we should commit to the brace-syntax from magrittr or grab the result in an extra supressWarnings() for the xml_nodes_to_lints() tests.

The former would not be compatible with the native pipe once we drop support for R versions without it.

Bumping the minimum needed R version to 4.1 should be many years away, so that's not too worrisome.

That said, the brace syntax is not too straightforward to reason about, and so maybe it's better to avoid it. Thus, I have removed the brace-syntax, and have instead used temp variables for the tests in question.

[MichaelChirico]

I found myself ready to suggest nesting the expectations instead & then remembered that's basically what we just undid by removing magrittr :)

Someday we'll depend on R>4.1 & then we can use |>....

In the meantime, there may be some cases where 2-nesting looks fine:

expect_warning(
  expect_identical(ls(lintr:::load_cache(file = f1, path = d1), character()),
  "Could not load cache file",
  fixed = TRUE
)

up to you whether / which cases you agree

[IndrajeetPatil]

TBH, I personally prefer to keep each expectation separate. Nested expectations are much harder to reason about.