keyword_quote_linter for unnecessary quoting

[MichaelChirico]

Continuing on with #884.

There are a few customizations/options we could add, LMK if we should do those here or in follow-up issues.

[codecov-commenter]

Codecov Report

Merging #2030 (e30fa49) into main (788bdca) will increase coverage by 0.00%.
The diff coverage is 100.00%.

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

@@           Coverage Diff           @@
##             main    #2030   +/-   ##
=======================================
  Coverage   99.61%   99.62%           
=======================================
  Files         115      116    +1     
  Lines        4983     5049   +66     
=======================================
+ Hits         4964     5030   +66     
  Misses         19       19           

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

[MichaelChirico]

Draft: review #2031 first

[AshesITR]

I was stumped and debugged.
Turns out the actual problem is xml_child() is not vectorized for nodesets:

The correct code should be

assignment_to_string <- vapply(
  assignment_expr, 
  function(x) xml_name(xml2::xml_child(x)) == "STR_CONST", 
  logical(1L)
)

[MichaelChirico]

Thanks for checking. Are you suggesting we shouldn't use xml_children()? Your workaround looks a lot clunkier given that the code already works as intended with xml_children()

[AshesITR]

I'm suggesting to fix the comment.
It's actually not <= 1 but = 1 children guaranteed by the XPath, making xml_children() correct.
The empty case (0 elements) occurs when there are no matches for the XPath, where xml_children() also behaves correctly by returning xml_missing().

[MichaelChirico]

oh, but that's what I meant -- 1 if there's a match, 0 if not, so <= 1, and it works in both cases. Let me reword for clarity.

[AshesITR]

The part about xml_child is wrong. It's simply not vectorized over nodesets so it would return the first child of the first match instead of the first child for each element in the resultset, and error if the resultset is empty.

[MichaelChirico]

Ohh I get you now. even better.

[AshesITR]

I wonder how that slipped...

[MichaelChirico]

I must have forgotten to roxygenize() after adding the examples in the other PR...