refactor spaces_inside_linter to use xpath

[MichaelChirico]

Part of #1160

@AshesITR PTAL -- our column numbers are off again here.

match_after_end will work for the LHS case ([ or () but not the RHS case.

Two options:

1. Refactor xml_nodes_to_lints() again -- maybe match_after_end ➡️ match_column = c("center", "after", "before"), where "before" works for the ]/) cases here?
2. Just don't use xml_nodes_to_lints() here
3. other ideas?

[AshesITR]

Is it possible to solve with the current implementation of xml_nodes_to_lints() if the two XPaths are split into separate nodesets?
Also, wondering what column results in optimal usability. If the new columns are better, I'm also open to changing the expected results.

[AshesITR]

Okay, I pondered this for a while, with two observations:

1. the old locations are good, they shouldn't change
2. xml_nodes_to_lints() is probably not useful here.

By the way, I have issues with the print.lints() method on this branch and maybe on others:

> lint(text = "a( 1 )")
<text>:1:2: style: Do not place spaces around code in parentheses or square brackets.
NA
 ^
<text>:1:6: style: Do not place spaces around code in parentheses or square brackets.
NA
     ^

[MichaelChirico]

xml_nodes_to_lints() is probably not useful here.

that means to abandon the xpath approach, or to roll our own conversion from xml match --> Lint?

[AshesITR]

I'd use a custom lapply().
How to the two versions compare performance-wise?
If the old implementation is faster, no need to switch IMO. It would then be better to just compact() the linters output so we get the desired consistency.

[MichaelChirico]

I'll test performance out, but i also think the xpath version is a fair amount more readable.

[AshesITR]

I agree to that.

[MichaelChirico]

(hopefully the adjustments needed to avoid xml_nodes_to_lints() don't tank the readability 😄)

[MichaelChirico]

PTAL. I still used xml_nodes_to_lint()... the positions look fine to me:

both land you where you can delete the whitespace which is what's important.

On main, the difference is both markers are to the left of the whitespace, so user consistently deletes forward, whereas here, we land to the left of opening whitespace and to the right of closing whitespace.

I think that's fine -- in both cases there's a single key to press on many keyboards (or a key combo to press for forward-delete on laptops). If anything, if we insisted on consistency, I'd say it's better to be to the right in both cases since backwards-delete is usually easier.

[AshesITR]

Being to the left also causes the print() to look nicer:

> lint(text = "a( 1 )", linters = spaces_inside_linter())
<text>:1:3: style: Do not place spaces around code in parentheses or square brackets.
a( 1 )
  ^
<text>:1:5: style: Do not place spaces around code in parentheses or square brackets.
a( 1 )
    ^

[MichaelChirico]

Hmm I see what you mean.

OTOH, the source marker is maybe not very nice for a wider expression:

This branch is also way faster -- 20x vs. main (see details)

On balance, between (1) the improved message (now says "before" when print() lands on the bracket); (2) consistently press one button (Backspace or Delete); (4) improved efficiency; and (3) the easier maintenance & readability, I still think this approach is preferable.

benchmarked on tools/R/QC.R, isolating the `get_source_expressions` overhead:

library(lintr)
l = spaces_inside_linter()
f = "~/svn/R-devel/src/library/tools/R/QC.R"
e = get_source_expressions(f)
system.time(lapply(e$expressions, l))
# to check overlap
write.csv(as.data.frame(lint(f, l)), "branch.csv")

[AshesITR]

It would be awesome if we modified the ranges to include the entire whitespace and we can add a preceding-siblling::*[1] to the OP-RIGHT-PAREN path to arrive at the 1 in this case.
Ideal output imo would be

a[   1   ]
  ^~~

a[   1   ]
      ^~~

The speedup is awesome. We should definitely use XPath then, just fine-tune the locations a bit.

[MichaelChirico]

I think that's a separate PR, WDYT?

Now the refactor handles XML logic and fixes the no-match output, with tiny nudge to source markers (in the right direction I think).

Follow-up can further improve lint metadata.

[AshesITR]

Filed #1205