User-defined S3 methods & object_name_linter

[russHyde]

This has been raised in a few issues (which I think have all been closed). But it isn't an issue that has really been solved.

If a newly minted package defines a generic and an S3-method based on that generic:

#' drink_me
#' @description empty
#'
#' @export
drink_me <- function(x, ...) {
  UseMethod("drink_me")
}

#' drink_me for most things
#' @export
drink_me.default <- function(x, ...) {
  1
}

#' drink_me for lists
#' @export
drink_me.list <- function(x, ...) {
  NULL
}

#' drink_me for data.frames
#' @export
drink_me.data.frame <- function(x, ...) {
  NULL
}

then, IMO, the expected behaviour when running lint_package(linters = object_name_linter("snake_case")) is for no lints to be thrown. Only the name of the generic should be subject to the snake-case linter.

But the .<className> suffix throws a lint because of there being multiple dots in the drink_me.data.frame

To reproduce

• open rstudio
• make new package ~/temp/temppkg using rstudio package template
• delete the NAMESPACE
• add the above code to drink.R
• document()
• Use devtools::load_all("path/to/lintr") to load current lintr master
• lint_package(linters = object_name_linter("snake_case"))
• Note the lint on drink_me.data.frame
• ... then
• load the temp package using devtools::load_all()
• Run lint_package again
• note that the lints are present whether or not the package-under-development is loaded.

This is an issue that arose while experimenally running lint_package(linters = object_name_linter(c("snake_case", "camelCase", "symbols"))) on {targets} (https://github.com/ropensci/targets)

[russHyde]

Cross-ref #223

[russHyde]

Cross-ref #182 (Jim states that it's not possible to determine if a given function is an S3 method without loading all possible S3 generics)

[russHyde]

Could a simple solution be let the user provide a set of allowed-suffixes for function-names? eg, here (".default", ".data.frame", ".list")

[AshesITR]

Is the drink_me generic correctly detected?
If so, what about simply allowing all function names that are prefixed by <generic>.?
Since the class() attribute can be pretty much anything, I think this wouldn't produce false negatives.

[russHyde]

I'm not sure what you mean by correctly detected. The resulting NAMESPACE file looks like this:

# Generated by roxygen2: do not edit by hand                                                                                                                                           
                                                                                                                                                                                       
S3method(drink_me,data.frame)                                                                                                                                                          
S3method(drink_me,default)                                                                                                                                                             
S3method(drink_me,list)                                                                                                                                                                
export(drink_me)

[AshesITR]

I wonder if lintr recognizes it, i.e. if "drink_me" %in% generics after this:

https://github.com/jimhester/lintr/blob/180e1a198469210674c2d3755e7a3c38fa47b61d/R/object_name_linters.R#L55-L59

If not, I'd consider that a bug, since declared_s3_generics() should find it and then the following codes shouldn't produce a lint for the S3 methods, even if they have an "ugly" suffix.

Imagine defining

drink_me.mY_Ugly.Class.that_ignores_all_styleguides

Not even that should lint IMO.

[russHyde]

debug(lintr:::check_style)
lint_package(linters = object_name_linter("snake_case"))

# In check_style, drink_me.list, drink_me.default and drink_me.data.frame are found not to be "conforming"
# The values in `possible_s3` are
possible_s3
        generic  method
1      drink_me default
2      drink_me    list
3 drink_me.data   frame 

[russHyde]

Where

possible_s3 <- re_matches(
      nms[!conforming],
      rex(capture(name = "generic", something), ".", capture(name = "method", something))
    )

Should the generic or the method be greedy in this regex?
At present the generic is greedy and matches everything up to the final "."

Assuming the generic name does not have a dot in it, we can do:

rex(capture(name = "generic", except_some_of(".", type="greedy")), ".", capture(name = "method", something))

This forces the user to disallow "." in the generic name; which would (detrimentally) prevent them from adding as.data.frame.MyClass

[AshesITR]

I think we should make the generic explictly and greedily match exactly what is in the generics vector.
This way only matches will be attempted for actually existing generics, giving precedence to longer generic names.

A useful test case might be your suggestion

as.data.frame.MyClass

This should match the generic as.data.frame, even if as is also registered as a generic.
In contrast,

as.data.fame.MyClass

(typo intended)
should match as as a generic candidate if we have that in the list of generics.