first pass at implementing a backport_linter #622

MichaelChirico · 2020-11-29T07:14:07Z

Closes #506

For now it's just a simple version. We simply gather all* the new functions mentioned in NEWS.3 and NEWS and check if those names are used at all in the script. My guess is that the chance of namespace collisions is low for base functions, so I didn't bother with any namespace qualification complexity.

I think adding a check for raw strings is doable in this PR, the only thing is I think we need to do a skip for related tests right? Since we rely on parse() to work.

My gut says checking R-level env variables which are added over time is not likely to bear much fruit -- a test of this would be to identify a few and cran-search whether any are used in CRAN packages.

Checking argument consistency I would say is beyond scope for now. In particular, note how complicated this will be for use cases involving lapply & co. I would wait on seeing if there's FR for that first 😃

I did this by eye so there's a chance I missed something

lintr-bot · 2020-11-29T07:17:58Z

R/backport_linter.R:48:11: style: Use <-, not =, for assignment.

backports = list(
          ^

R/backport_linter.R

AshesITR · 2020-11-29T10:02:51Z

R/backport_linter.R

+#' @export
+backport_linter <- function(r_version = "4.0.3") {
+  function(source_file) {
+    if (is.null(source_file$xml_parsed_content) || r_version >= "r-devel") return(list())


Why is there a lexicographic comparison to "r-devel"?
Both "r-release" and "r-oldrel" would trigger this.

Maybe I should just leave this as "r" -- I don't think we want to support tracking devel dependencies, I just wanted something that would be bigger than all R version numbers (at first I used "999999" as the name, "r-devel" felt better but you raise a good point)

Couldn't we consult names(backports) to find whether the selected version is supported?
The highest of those versions wouldn't cause lints anyway so we can tail() it to obtain a definitive list of R versions that can result in lints if I'm not mistaken.

if so we'd have to include in the list all released versions (e.g. 3.5.3).

And sorry I'm just waking up, I'm not following your second point

Good point.
Wouldn't we never lint if r_version is at least getRversion()?
i.e. if (r_version >= getRversion()) return(list()).
That way if r_version is older than the current environment, backport problems can be detected.
If it isn't there are no backports that can be used from the current R version which would be a problem in r_version, right?

I think that's basically right, but I do think it's possible to run this on a machine that has a different environment than required. Say I work on two machines and the one I'm using today has an old version. We wouldn't want to skip then necessarily right?

On the old version, the used backports would either break the code completely (which you'd notice), or the backports were already safely ported to your old version. No?

That's if we run tests, but lintr is static analysis so we could in principle run it on a different machine that doesn't execute code.

If that machine is running for a bigger library of R code (say a local CRAN), there may be different code requirements.

I think it's basically an edge case... but taking a step back, the goal of this is to get around/change the r_version >= "r" part? I'm happier with that now than I was with r_version >= "r-devel"

I agree. Let's just leave it as-is for now.
Maybe later become smart about what r_versions are found in backports and thus will potentially cause lints.

R/backport_linter.R

AshesITR · 2020-11-29T10:12:51Z

R/backport_linter.R

@@ -0,0 +1,83 @@
+#' @describeIn linters that checks for usage of unavailable functions. Not reliable for testing r-devel dependencies.
+#' @export
+backport_linter <- function(r_version = "4.0.3") {


Can the default auto-detect, e.g. getRversion()?
Or even better for Packages, get the R version from the Depends field in DESCRIPTION? (this might be worth a separate PR, also suppressing lints for stuff imported from {backports})

good idea, and a good direction for follow-up. Actually I think we can go beyond backports (there may be other backport packages) and try to check namespace imports and/or internally defined functions & skip those. New argument skip= defaulting to infer_skips where we apply some logic.

I suggest using except as the argument name - it's already used in missing_argument_linter with the same purpose:

#' @param except a character vector of function names as exceptions.

renkun-ken · 2020-11-29T11:53:26Z

R/backport_linter.R

@@ -0,0 +1,83 @@
+#' @describeIn linters that checks for usage of unavailable functions. Not reliable for testing r-devel dependencies.
+#' @export
+backport_linter <- function(r_version = "4.0.3") {


Should the default r_version be the current R version?

If a package author wants to use this linter, then one must specify the r_version to be consistent with that appears in the package DESCRIPTION?

Now I used getRversion(), I guess we could make it guess_r_version() & check the description first before defaulting to getRversion()?

R/backport_linter.R

tests/testthat/test-backport_linter.R

R/backport_linter.R

…ackports-warning

AshesITR · 2020-11-29T20:45:46Z

R/backport_linter.R

+backport_linter <- function(r_version = getRversion()) {
+  function(source_file) {
+    if (inherits(r_version, "numeric_version")) r_version <- format(r_version)
+    if (r_version < "3.0.0") {


We should use numeric_version comparison here, else it will bite us when R 10.0.0 rolls out ;)

haha that can be a problem for our future selves -- after 2050 at the current rate 😸

I prefer this way because the test below makes the same assumption by doing r_version < names(backports)

So really this assumption is backed into names(backports) as well. Maybe by R 10.0 R will allow objects to have objects as names like python

MichaelChirico · 2020-11-29T20:54:57Z

BTW @AshesITR we should come up with a procedure for new linters & whether/how to include them as default linters / whether/how to include them in the GH action for linting lintr...

e.g. we could make that part of a separate PR, but in this PR we could add it in .lintr with a TODO & follow-up issue?

AshesITR · 2020-11-29T21:48:02Z

What do you mean?
add backports_linter to our .lintr now with a TODO for adding it to default linters?
The GH action (and travis) currently use our .lintr and I think that's a good way to have it because everyone can run lintr::lint_package() locally and expect the same results.

IMO adding a linter to the default linters should always be a separate issue.

MichaelChirico · 2020-11-29T22:47:30Z

Do we only lint lintr itself with default linters?

MichaelChirico · 2020-11-30T01:26:33Z

Hmm I want to include a test for raw strings, however, there's an issue:

The parse data for raw strings is not returned correctly in 4.0.0 (only in 4.0.1), so the test can't work on 4.0?

I'll have to think about it more; for now, let's merge this and file that as a follow-up?

AshesITR · 2020-11-30T11:39:33Z

Sounds good to me.
WRT linting lintr with only the default linters: I don't mind having more strict linting in lintr itself than default.

renkun-ken

LGTM

first pass at implementing a backport_linter

dbff7ca

MichaelChirico requested a review from renkun-ken November 29, 2020 07:14

need Collate order

8662f64

good job lintr bot

7fcda6a

AshesITR requested changes Nov 29, 2020

View reviewed changes

renkun-ken reviewed Nov 29, 2020

View reviewed changes

R/backport_linter.R Show resolved Hide resolved

MichaelChirico added 2 commits November 29, 2020 12:48

incorporate some feedback

99f03f0

Merge branch 'master' into backports-warning

e3b7efe

AshesITR reviewed Nov 29, 2020

View reviewed changes

R/backport_linter.R Outdated Show resolved Hide resolved

MichaelChirico added 2 commits November 29, 2020 13:58

rename to devel per good suggestion

2e29980

Merge branch 'master' into backports-warning

3bb6c3c

AshesITR reviewed Nov 29, 2020

View reviewed changes

tests/testthat/test-backport_linter.R Outdated Show resolved Hide resolved

AshesITR reviewed Nov 29, 2020

View reviewed changes

R/backport_linter.R Outdated Show resolved Hide resolved

MichaelChirico added 3 commits November 29, 2020 15:34

update tests

51173cc

add test of really old version; remove test of r-devel

d6ca8d5

Merge branch 'backports-warning' of github.com:jimhester/lintr into b…

33c952b

…ackports-warning

AshesITR previously approved these changes Nov 29, 2020

View reviewed changes

renkun-ken previously approved these changes Nov 30, 2020

View reviewed changes

Merge branch 'master' into backports-warning

038df29

MichaelChirico dismissed stale reviews from renkun-ken and AshesITR via 038df29 November 30, 2020 15:57

AshesITR previously approved these changes Nov 30, 2020

View reviewed changes

Merge branch 'master' into backports-warning

e9a54d4

MichaelChirico dismissed AshesITR’s stale review via e9a54d4 December 1, 2020 22:01

AshesITR previously approved these changes Dec 1, 2020

View reviewed changes

Merge branch 'master' into backports-warning

c653b51

MichaelChirico dismissed AshesITR’s stale review via c653b51 December 4, 2020 05:46

Merge branch 'master' into backports-warning

3dad15a

AshesITR approved these changes Dec 4, 2020

View reviewed changes

MichaelChirico added 3 commits December 4, 2020 15:50

Merge branch 'master' into backports-warning

b141e3a

Merge branch 'master' into backports-warning

f893126

Merge branch 'master' into backports-warning

ca8c49a

AshesITR merged commit 9104a00 into master Dec 5, 2020

AshesITR deleted the backports-warning branch December 5, 2020 21:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

first pass at implementing a backport_linter #622

first pass at implementing a backport_linter #622

MichaelChirico commented Nov 29, 2020

lintr-bot commented Nov 29, 2020

AshesITR Nov 29, 2020

MichaelChirico Nov 29, 2020

AshesITR Nov 29, 2020

MichaelChirico Nov 29, 2020

AshesITR Nov 29, 2020

MichaelChirico Nov 29, 2020

AshesITR Nov 29, 2020

MichaelChirico Nov 29, 2020

AshesITR Nov 29, 2020

AshesITR Nov 29, 2020

MichaelChirico Nov 29, 2020

AshesITR Nov 29, 2020

renkun-ken Nov 29, 2020

MichaelChirico Nov 29, 2020

AshesITR Nov 29, 2020

MichaelChirico Nov 29, 2020

MichaelChirico commented Nov 29, 2020 •

edited

Loading

AshesITR commented Nov 29, 2020

MichaelChirico commented Nov 29, 2020

MichaelChirico commented Nov 30, 2020

AshesITR commented Nov 30, 2020

renkun-ken left a comment

first pass at implementing a backport_linter #622

first pass at implementing a backport_linter #622

Conversation

MichaelChirico commented Nov 29, 2020

lintr-bot commented Nov 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelChirico commented Nov 29, 2020 • edited Loading

AshesITR commented Nov 29, 2020

MichaelChirico commented Nov 29, 2020

MichaelChirico commented Nov 30, 2020

AshesITR commented Nov 30, 2020

renkun-ken left a comment

Choose a reason for hiding this comment

MichaelChirico commented Nov 29, 2020 •

edited

Loading