Equals na comment

[MichaelChirico]

Closes #545

Beyond just skipping usages which might be found in comments, I also took the opportunity to extend the logic of the linter:

• NA == x is now matched (before, it was just x == NA; this came for free with the move to xpath)
• x != NA is also matched. It would be trivial to extend this to other comparators (>|>=|<|<=), but I wasn't sure that would gel with the name of the linter (not equals is similar enough I think)
• This will also now match multiline usages like x\n==\nNA (again, for free with the move to xpath).

I also added a helper function for extracting Lints from their xml nodes. This can likely be re-used in a few places.

[AshesITR]

The logs complain about mising xml_nodes_to_lint().
I don't see it in the commits.

Also note you'll probably need to use @include xml_nodes_to_lint.R in all the files that use the function in order to have roxygen compute the correct collate order.

[AshesITR]

Thanks!

[MichaelChirico]

The failure on CI is due to the following:

lintr::expect_lint("\\", rex::rex("unexpected input"), lintr::equals_na_linter)
# Error in UseMethod("xml_find_all") : 
#   no applicable method for 'xml_find_all' applied to an object of class "NULL"

The issue is that equals_na_linter receives an object with length(source_file$parsed_content)=1 but !length(source_file$xml_parsed_content).

Is that expected behavior? Till now, I've assumed length(source_file$parsed_content) > 0 implies the XML content would be there.

In this case, since the code doesn't in fact parse:

parse(text = "\\")
# Error in parse(text = "\\") : <text>:1:1: unexpected input
# 1: \
#     ^

I am surprised source_file$parsed_content is nonempty.

[AshesITR]

This is due to #560.
The failing code produces an invalid XML but getParseData works on it.

[MichaelChirico]

I see... that means we have to keep parsed_content around in order to make good error messages? Or can we NULL it out before passing it on to be linted?

If not, I think we'd have to do something like this in all XML-based linters:

# this is in several linters already, e.g. seq_linter, missing_argument_linter, namespace_linter
if (!length(source_file$parsed_content) || is.null(source_file$xml_parsed_content))

pretty awkward IMO!

[AshesITR]

I agree that this double check looks a bit weird.
Maybe we should open a separate issue to adjust the behaviour and all current checks of this kind?

[MichaelChirico]

OK moved back to WIP, I'll update it once #609 is merged.

[MichaelChirico]

OK this should be ready to go now. Passing locally.

[MichaelChirico]

TIL sQuote(x, '"') is an R 3.6 addition. fun. Will rewrite it in utils.

[AshesITR]

Nice, thanks!