adokter / bioRad

R package for analysis and visualisation of biological signals in weather radar data
http://adokter.github.io/bioRad
Other
29 stars 16 forks source link

move helper functions to zzz.R #586

Closed PietrH closed 1 year ago

PietrH commented 1 year ago

Currently unexported helper functions tend to reside in the same file as the function they are used in. I propose moving all the helpers into zzz.R

Functions with @keywords internal are present in:

And as expected in zzz.R

Are there disadvantages to moving them?

When covr reports test coverage, it does so by lines, so by moving the helpers into their own file, we can also split up helper/function coverage, not that we shouldn't cover both of course.

bart1 commented 1 year ago

I could live with either, the reason for keeping would be that it is nice to have the helper function there if you are working on the main function. It makes it easier to reference back and forward

PietrH commented 1 year ago

While I appreciate the convenience, it does mean you need to know a helper exists in order to reuse it. Some are pretty general, and I could certainly see them being used over multiple functions. For example:

#' Match a set of regular expressions to a list of files
#'
#' Match a set of regular expressions to a list of files and return those
#' filenames that comply to any of the provided regular expressions. This
#' function basically wraps a grep to make it work on vectors by combining the
#' vector of regex options as possible options.
#'
#' @param file_list character vector. Haystack of filenames/filepaths.
#' @param regex_list character vector. Needle of regular expressions to which
#'   filenames should comply.
#'
#' @return character vector. Subset of filenames from the file_list that comply
#'   to the provided regular expressions in regex_list.
#' @keywords internal
match_filenames <- function(file_list, regex_list) {
  grep(paste(regex_list, collapse = "|"), file_list, value = TRUE)
}

Or this one I added recently:

#' extract strings from a vector using regex, analog to stringr::str_extract
#'
#' @param string Input vector. A character vector.
#' @param pattern Regex pattern to look for
#' @param ... passed on to `regexpr()`
#'
#' @return A character vector with matches only, possibly of different length as
#'   `string`
#' @keywords internal
extract_string <- function(string,pattern,...) {
  regmatches(string,
             m = regexpr(
               pattern = pattern,
               text = string,
               ...
             ))
}
bart1 commented 1 year ago

I think for these general ones your definitely right! Maybe make it dependent how likely reuse is?

PietrH commented 1 year ago

That seems reasonable to me

peterdesmet commented 1 year ago

Generic helper functions are indeed best moved to same file. Specific ones can remain in the same file.

Can I also suggest to rename zzz.R to utils.R (based on this blog post). Makes it nicer to split that file later, e.g. in a utils-suggests.R for all helper functions related to nudging users to install suggested packages.

PietrH commented 1 year ago

Fine by me. I'll get started on this.