Create list2df_linter()

[Bisaloo]

Fix #2596

[MichaelChirico]

quick feedback: let's name it list2df_linter(). suppose there are other lint rules we come up with in the future that result in the recommendation to use list2DF() -- we'd still want them in this same linter.

similarly, we have nzchar_linter() and lengths_linter() (among others) where the linter name is derived from the recommended function.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.64%. Comparing base (8913141) to head (7dbcec4).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2834   +/-   ##
=======================================
  Coverage   99.64%   99.64%           
=======================================
  Files         126      127    +1     
  Lines        6960     6979   +19     
=======================================
+ Hits         6935     6954   +19     
  Misses         25       25           

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

Had a go at refining the wording here too, PTAL

[MichaelChirico]

Strange, I get this error in r-devel

lint('utils/R/str.R', list2df_linter())
# Error in `lint()`:
# ! Linter `linter()` failed in /home/michael/svn/R-devel/src/library/utils/R/str.R:
# Caused by error in `xml_nodeset()`:
# ! Expecting an external pointer: [type=NULL]
# Run `rlang::last_trace()` to see where the error occurred.

[MichaelChirico]

Looks like it's due to a lambda in do.call():

lint("do.call(function(x) x, l)\n", list2df_linter())
# Error in `lint()`:
# ! Linter `linter()` failed in /tmp/RtmpTKzzIM/file1393690d1c43:
# Caused by error in `xml_nodeset()`:
# ! Expecting an external pointer: [type=NULL]
# Run `rlang::last_trace()` to see where the error occurred.

[Bisaloo]

The edits look good to me. Thanks!

I can indeed still reproduce the do.call() issue with the latest R-devel but it seems unrelated to this PR. What would you recommend here?

[MichaelChirico]

it seems unrelated to this PR

Hmm, how do you figure? it's being generated by {lintr} code and only from this new linter.

[Bisaloo]

Hmm, how do you figure? it's being generated by {lintr} code and only from this new linter.

No, you're right. For some reason, I was under the impression this happened at the parsing stage, before reaching lintr, but it was incorrect.

I have added a test for this, fixed the immediate issue, and added an extra !is.na() before the equality testing to avoid similar issues with more complex expressions I didn't imagine.

[MichaelChirico]

right now, the linter will land on the first argument, is that the best place? Or should we include the whole do.call expression? (not immediately clear to me, though I lean towards the latter)

do.call(cbind.data.frame, ...)
        ^ ~ current    |
^         alternative        |

[Bisaloo]

I agree. I have done this by adding an xml_find_all(, "./parent::expr") before returning. Please let me know if this is not the preferred option.

[MichaelChirico]

let's use xml_parent() in that case

#2718 (comment)

[Bisaloo]

Hi @MichaelChirico, since I have made a couple of (minor) changes since you approved, I am not completely sure if I should go ahead and merge this.