Add library call linter

[nicholas-masel]

I've got a use case where I need all library() calls to be at the top of the script so the search path is consistent across the entire script.

I will incorporate this into the linting for the logrx package, but thought others might want this as well, so better in lintr than buried in a logging package. I hope you all agree.

[nicholas-masel]

I'm not sure why the lint is returning multiple and causing the tests to fail. When I run this on my side it returns one and everything is running cleanly.

library(lintr)

lint(
  text = c("library(dplyr)", "print('test')", "library(tidyr)"),
  linters = library_call_linter()
)
#> <text>:3:1: warning: [library_call_linter] Move all library calls to the top of the script.
#> library(tidyr)
#> ^~~~~~~

^{Created on 2023-07-28 with reprex v2.0.2}

[AshesITR]

AFAIK the last() predicate refers to the last element that satisfies a condition within each node.
That means it probably doesn't do anything in the XPath, because all the library calls are contained inside their own expr element.

[nicholas-masel]

AFAIK the last() predicate refers to the last element that satisfies a condition within each node. That means it probably doesn't do anything in the XPath, because all the library calls are contained inside their own expr element.

@AshesITR Thanks! I'll try (//element[@name='D'])[last()] as this post suggests to see if that gives me the last node, rather than the last element within each node.

I'm still unclear why it returns the expected result on my end and fails when running through ci/cd here. It makes it so the only way I can test is to push updates and check the actions, which is probably destroying your email inbox with fail messages. Sorry about this.

[MichaelChirico]

probably destroying your email inbox with fail messages

FWIW I don't get those. only the code coverage bot comments when CI passes.

[nicholas-masel]

probably destroying your email inbox with fail messages

FWIW I don't get those. only the code coverage bot comments when CI passes.

Good to know, thanks! Now I don't feel so bad hacking through it this way :)

[MichaelChirico]

I'm still unclear why it returns the expected result on my end and fails when running through ci/cd here

could be an R version thing? there are subtle differences in the XML structure between versions sometimes. To help, you could add writeLines(as.character(XML)) to facilitate print debugging.

[nicholas-masel]

writeLines(as.character(XML))

Great idea! I'm not clear how would I retrieve this from the action? Can I access this somehow after the action is complete?

[nicholas-masel]

writeLines(as.character(XML))

Great idea! I'm not clear how would I retrieve this from the action? Can I access this somehow after the action is complete?

@MichaelChirico nvm! This defaults to standard out!

[codecov-commenter]

Codecov Report

Merging #2027 (34b899f) into main (37e9c39) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 34b899f differs from pull request most recent head 4a0754f. Consider uploading reports for the commit 4a0754f to get more accurate results

@@           Coverage Diff           @@
##             main    #2027   +/-   ##
=======================================
  Coverage   98.55%   98.56%           
=======================================
  Files         113      114    +1     
  Lines        4994     5011   +17     
=======================================
+ Hits         4922     4939   +17     
  Misses         72       72           

Files Changed	Coverage Δ
R/library_call_linter.R	100.00% <100.00%> (ø)

[AshesITR]

Good work.
Don't forget to mention yourself in a NEWS bullet.

[MichaelChirico]

this counts as readability to me too

[nicholas-masel]

This one is updated.

[MichaelChirico]

You might want to add an exception for library() calls inside functions -- e.g. for libraries calling library() on behalf of the user. The current logic seems tailored to scripts, but the linter will end up being used in packages too.

It would also be easy to consider require() the same as library() for this linter.

[nicholas-masel]

You might want to add an exception for library() calls inside functions -- e.g. for libraries calling library() on behalf of the user. The current logic seems tailored to scripts, but the linter will end up being used in packages too.

It would also be easy to consider require() the same as library() for this linter.

@MichaelChirico Would you be open to completing this PR as is and adding this as a new issue? I'm happy to continue to work on it to add these features, but it would allow me to get this functionality in so I can have another package use it for it's upcoming release.

[MichaelChirico]

SGTM, please help to file follow-up issues