Violation of least astonishment, asymmetry of behavior or lack of documentation in expect_lint_free()

[stefanoborini]

Related to #406

By default and with no mention in the documentation the expect_lint_free function assume it's running on CRAN, unless told otherwise, and will say absolutely nothing about it:

> lintr::expect_lint_free
function (...) 
{
    testthat::skip_on_cran()
    lints <- lint_package(...)
    has_lints <- length(lints) > 0 
    lint_output <- NULL
    if (has_lints) {
        lint_output <- paste(collapse = "\n", capture.output(print(lints)))
    }   
    result <- testthat::expect(!has_lints, paste(sep = "\n", 
        "Not lint free", lint_output))
    invisible(result)
}
> testthat::skip_on_cran
function ()  
{
    if (identical(Sys.getenv("NOT_CRAN"), "true")) {
        return(invisible(TRUE))
    }   
    skip("On CRAN")
}

In other words, its default assumes that you are CRAN, unless you specifically say otherwise. This is a least astonishment violation, an asymmetry in behavior between lint_package() and expect_lint_free(), and lack of documentation clarity.

This is mostly a testthat issue, as the default should be to check for a positive confirmation that you are on CRAN, not a negative one. It has, however, a direct influence on lintr behavior.

[jimhester]

If you want to submit a PR to update the documentation you are welcome, otherwise I am closing this issue as it is working as intended.

[stefanoborini]

I completely disagree. It's not working as intended and it should not be closed because it's an issue that must be resolved. Otherwise you would not ask for a PR.

[infotroph]

@stefanoborini My knowledge here is limited, but to my eye NOT_CRAN is a ~standard approach in the R testing world and therefore not astonishing: The maintainers of CRAN want your code to behave by default in a way that complies with CRAN policies, so needing to ask specially for the non-CRAN behavior is intended.

(My understanding is that the "NOT_CRAN" convention arose because the CRAN maintainers have extended the 'should behave this way by default' idea into a principle of not providing any particular test your code can use to determine for sure that it is on CRAN -- i.e. there's no equivalent of Travis's CI variable -- so instead we invert it by setting an environment variable that probably won't be set on CRAN. But it probably also won't be set on a random user's machine, reducing the temptation for someone to use it as a way of subverting the CRAN check process).

[stefanoborini]

@infotroph nothing in the name "expect_lint_free" function name or documentation hints at the possibility that this function will be a no-op unless some specific environment characteristics exist that are never mentioned anywhere in the documentation of the whole package or of the function itself.

I expect a linter to lint my code. Not to lint my code if the planets align.