Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add/export a readable wrapper for testing whether a linter is on an expression or the whole file #921

Closed
MichaelChirico opened this issue Mar 11, 2022 · 3 comments · Fixed by #1073
Milestone

Comments

@MichaelChirico
Copy link
Collaborator

MichaelChirico commented Mar 11, 2022

We've got (minor perturbations of) this little idiomatic piece at the top of basically every linter:

if (!is.null(source_file[["file_lines"]])) {
# abort if source_file is entire file, not a top level expression.
return(NULL)

if (is.null(source_file$file_lines)) return(list())

if (is.null(source_file$full_xml_parsed_content)) return(list())

This is too obscure to a first-time reader. We should add a wrapper of this expression that makes it clear what's being done/why, and also export it for use in custom downstream linters.

@AshesITR
Copy link
Collaborator

Good idea. is_complete_file(source_file, require_xml = FALSE)?
I remember the is.null() check on xml_parsed_content being used in some places specifically because it can happen that xmlparsedata fails or returns invalid XML.

Impl would be something like

if (require_xml) {
  !is.null(source_file[["full_xml_parsed_content"]])
} else {
  !is.null(source_file[["file_lines"]])
}

@MichaelChirico MichaelChirico added this to the 3.0.0 milestone Mar 22, 2022
@MichaelChirico
Copy link
Collaborator Author

Twice now I've been bitten by the require_xml part so this helper will do wonders for developers who don't understand the arcana of xml being sometimes present, sometimes not.

OTOH, maybe we could add a "dummy tree" when parsing fails and we can't add a real AST? Something as simple as

<xml>
<exprlist>
  <COMMENT>Parsing this file failed to generate a valid AST</COMMENT>
</exprlist>

Currently the failure mode is highly obscure:

══ Failed tests ══════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
── Error (test-error.R:46:3): returns the correct linting ───────────────────────────
Error in `UseMethod("xml_find_all")`: no applicable method for 'xml_find_all' applied to an object of class "NULL"
Backtrace:
    ▆
 1. └─lintr::expect_lint("\\", rex(expected_message)) at test-error.R:46:2
 2.   └─lintr::lint(file, ...) at lintr/R/expect_lint.R:56:2
 3.     ├─lintr::flatten_lints(linters[[linter]](expr)) at lintr/R/lint.R:113:8
 4.     │ ├─base::structure(flatten_list(x, class = "lint"), class = "lints") at lintr/R/utils.R:22:2
 5.     │ └─lintr::flatten_list(x, class = "lint")
 6.     │   └─lintr assign_item(x) at lintr/R/utils.R:43:2
 7.     │     └─base::inherits(x, class) at lintr/R/utils.R:35:4
 8.     └─linters[[linter]](expr) at lintr/R/lint.R:113:8
 9.       └─xml2::xml_find_all(xml, xpath) at lintr/R/if_else_match_braces_linter.R:36:4

@AshesITR
Copy link
Collaborator

Better use xml_missing() than a fake AST -- especially one that could be the result of valid code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants