Handle comparison to negative non-integer in expect_identical_linter

[Bisaloo]

Fix #2410

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.65%. Comparing base (fc2268f) to head (1b4b058).
Report is 7 commits behind head on main.

❗ Current head 1b4b058 differs from pull request most recent head 79334a8

Please upload reports for the commit 79334a8 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2411      +/-   ##
==========================================
+ Coverage   98.15%   98.65%   +0.49%     
==========================================
  Files         125      125              
  Lines        5738     5631     -107     
==========================================
- Hits         5632     5555      -77     
+ Misses        106       76      -30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MichaelChirico]

Suggested change

	and expr[NUM_CONST[contains(text(), '.')]]
	and expr/NUM_CONST[contains(text(), '.')]

nit: I've gravitated to this style to save a few characters (and also to avoid counting+matching nested ] visually)

[Bisaloo]

Where is the cut-off?

From what I understand, I could also write

expr[1][SYMBOL_FUNCTION_CALL[text() = 'c']]

as

expr[1]/SYMBOL_FUNCTION_CALL/text() = 'c'

[MichaelChirico]

I think the last condition belongs in [] -- otherwise it can get hard to visually parse operator presence of = vs. nearby logical operators like and/or. IINM occasionally we have to let /text() "out" e.g. to compare to the text() of another node, but it's best avoided.

We don't really have an XPath style guide, so these are just my thoughts from writing many of these, you or other lintr authors may disagree.

[MichaelChirico]

Hmm I saw this too -- so we haven't fully restored parity yet in this PR.

expect_equal(x, 1.02)     # no lint
expect_equal(x, c(1.02))  # no lint
expect_equal(x, -1.02)    # no lint
expect_equal(x, c(-1.02)) # lint!

Let's at least get parity in this PR. I would put all of the conditions on following-sibling::expr[] together btw, like:

following-sibling::expr[
  (
    expr[1]/SYMBOL_FUNCTION_CALL[text() = 'c']
    and expr/NUM_CONST[contains(text(), '.')]
  ) or (
    NUM_CONST[contains(text(), '.')]
  ) or (
    OP-MINUS ...
  )
]

Since NUM_CONST[contains(text(), '.')] is getting repeated so much let's extract it to a variable too to save space.

[MichaelChirico]

BTW, could you also test if the following lints?

expect_equal(x, c(1.02, y))

I think it's something to possible address in follow-up. I'm wondering if maybe it was intentional not to lint c(1.02, y) because there's some part of the expectation that uses an "exact" number so we "should" skip linting on the same principle as just 1.02? In which case we shouldn't lint 1.01-y either 🤔

@AshesITR any thoughts? This cut-out does feel a bit arbitrary, probably by necessity at some level. Wondering how to close the loop here for this PR, and what can be deferred to a more holistic follow-up.

[Bisaloo]

lintr::lint(
  text = "expect_equal(x, c(1.02, y))", 
  linters = lintr::expect_identical_linter()
)

^{Created on 2023-12-11 with reprex v2.0.2}

It doesn't lint.

[Bisaloo]

All tests are now passing. I'm not super happy about the complexity of this xpath but I can't think of something simpler at the moment. Please let me know if you have a better way in mind.

[Bisaloo]

#963 may be a good way to simplify the code eventually.

[AshesITR]

Is this case relevant in practice?

[Bisaloo]

I'm on the fence.

On the one hand, it should be caught be unnecessary_concatenation_linter() and converted to the standard case. On the other hand, it's not just a false negative; not treating this case would lead us to produce incorrect advice.