Decide on whether/when to use :: vs. @importFrom (vs. both); apply, follow/enforce

[brookslogan]

Currently we have a mix of :: and @importFrom that doesn't seem to follow a convention, or at least an explicit one. This makes it harder to check for namespace issues, and easier to get distracted by "false positives" when reading code that switches from code using :: to @importFrom[, and makes it harder to use test doubles correctly]. We should decide on a convention, apply it to existing code, and follow/enforce it moving forward.

A couple cases to consider:

• Typical stats conflicts (filter, lag):
  • If users copy-paste any of our code, using :: (in combination with @importFrom or by itself) will help them avoid errors. It also allows us to copy-paste code into examples and vignettes without having to think as much, and is friendlier to users that want to use a ::-only style.
  • These can evade convenient ways of checking for conflicts: e.g., in Km compactify rectify #101 we ran into a lag conflict where some devtools function (it was either devtools::check, devtools::run_examples, or both) would only flag/encounter the issue if starting from a fresh session that didn't have dplyr attached as a result of running other / the same dplyr commands.
• Binary functions:
  • We need to whether to @importFrom, %binop% = upstreampkg::`%binop% without exporting, or %binop% = upstreampkg::`%binop% with exporting.
• Tiny operations:
  • If we have any operations that are executed a huge number of times and do very little, maybe the overhead of :: might become relevant.
• [Things we want to make test doubles of (e.g., mocks):
  • We may want/need to wrap upstream functions in wrappers to be able to get test doubles to work how we want if we use ::, but not if we use @importFrom.]

[dajmcdon]

Just collecting some personal thoughts to add to the conversation

1. I suggest that we try to keep all the @importFrom tags in one place. This avoids doing it repeatedly (accidentally), though one could also check the NAMESPACE. A good location is probably epiprocess.R which documents the package.
2. The stats conflicts are annoying. I'd suggest we avoid importing things like dplyr::filter() for this reason and stick to the prefix.
3. Binaries like %>% should be reexported so that they can be used. This seems to be standard (the pipe). I'm not sure what others there are.
4. In @examples fields, we should call library(dplyr) or whatnot and then use the functions rather than prefixing.

[brookslogan]

Some comments:

1. (Context:) Up until now, I've favored putting @importFrom tags everywhere, repeatedly on purpose, in hopes that it will be easier to check that they are from the package we want and to reverse-find/highlight-symbol-at-point to check for completeness/correctness visually, without running package checks. Although I don't know if roxygen2 will actually catch if we try to include conflicting @importFrom statements.
2. +1 I also favor (or feel uncomfortable not using) :: in these (sometimes-)default package conflicts. In order to benefit from not importing here, it appears we have to be careful how we run the package checks; devtools seems eager to attach Imports: packages (plus epipredict Imports: some package that Depends: on dplyr for some reason and will attach it); otherwise the checks don't seem to complain!
3. There might be %||%, %@%. There is also the "polite fiction" of !! and !!!; these are defined as functions in rlang to be "polite" but aren't real operators; rlang's parser has to specially detect !'s in a row. There's also rlang's := and data.table's :=, which look like they might be real operators, but I'm not sure how we handle conflicts within the package and when exporting; this is where we might need to be careful.
4. Sounds reasonable, although I'm not sure if checks will detect if include library(dplyr) in one example and forget it in another example, and have a dplyr/stats conflict in that example.

[brookslogan]

More importantly on 4., if the @examples are long, the user would have to (not forget to) run the library chunk and the desired example chunk. It may still make sense to use :: everywhere to prevent this. If it's a default- or user-attache conflict then they may get incorrect results; if it's not a conflict, they'll get an error, need to go run the library chunk, then navigate to and re-run the desired example chunk.

[brookslogan]

Another note here: we need to @importFrom S3 generic functions that we provide methods for (r-lib/roxygen2#1412). Actual code could still be in either the @importFrom or :: style, although it might look a little strange for the latter. [Although @method tags --- which appear to be used already --- seem to work here to prevent the load errors. The difference is that document with @method will produce the proper S3 method export without the generic's import. I'm not sure whether one way or the other is "right" or not. The @importFrom could cause conflicts with unfortunately-named functions (e.g., filter) if we were using the non-:: approach with such functions. Testing again, @method tags don't seem to work by themselves.]

[dajmcdon]

The @method is useful for exporting an S3 method which has a dot in it. For example, in glmnet, cv.glmnet creates an object of class cv.glmnet, so the predict method is predict.cv.glmnet. Roxygen doesn't know what that is, so it needs

#' @method predict cv.glmnet
#' @export

Otherwise, the @export alone should be sufficient for Roxygen to determine that it's a method and write to NAMESPACE correctly.

[brookslogan]

(one more point here: mocking can have more issues / require more boilerplate if we use :: in internal package code)