SYNR-1301 check for version update

[kimyen]

@karawoo I fixed the bugs that you found. Could you review this code again? Thank you.

[kimyen]

This is great! Thanks @karawoo for the links. You pointed out that my code wasn't written according to best practices in R and provide documentation for me to understand why.

I do agree with using default values in the function signature over using dynamic lookup. However, with precision case, using a number causes lost of readability there. Someone else looks at my "new" code with precision as number would not understand that precision must be 1, 2, or 3, and what 1, 2, and 3 mean explicitly. Ideally, I would like precision to be an enum where the value can only be 1, 2, or 3. However, I don't think enum exist in R. A named list is the first that came to mind: precision <- c(MAJOR = 1, MINOR = 2, PATCH = 3) However, this gets me back to using dynamic lookup. Do you have any other recommendations such that I can keep the readability of what precision is, and what each valid value means?

[kimyen]

How about:

.simplifyVersion <- function(version, precision) {
  major <- 1
  minor <- 2
  patch <- 3
  if (!(precision %in% c(major, minor, patch))) {
    stop("Invalid precision: ", precision)
  }
  parts <- unlist(strsplit(version, "[.]"))
  parts[is.na(parts[1:precision])] <- "0"
  return(paste(parts[1:precision], collapse = "."))
}

[karawoo]

Someone else looks at my "new" code with precision as number would not understand that precision must be 1, 2, or 3, and what 1, 2, and 3 mean explicitly.

I'd normally document this type of thing in the roxygen comments for the function like so:

#' @param precision The level of precision to which version should be
#'   simplified. Can be 1, 2, or 3 corresponding to major, minor, or patch

but I see synapser is not using roxygen. Defining major, minor, and patch within the function seems fine to me. The named vector option also works (and doesn't have the dynamic lookup problem if you define it within the function):

.simplifyVersion <- function(version, precision) {  
  if (!(precision %in% c(MAJOR = 1, MINOR = 2, PATCH = 3))) {
    stop("Invalid precision: ", precision)
  }
  parts <- unlist(strsplit(version, "[.]"))
  parts[is.na(parts[1:precision])] <- "0"
  return(paste(parts[1:precision], collapse = "."))
}

Possibly even easier from the user perspective would be to accept precision as a string

.simplifyVersion <- function(version, precision) {
  precision_opts <- c("major", "minor", "patch")
  if (!(precision %in% precision_opts)) {
    stop("Invalid precision: ", precision)
  }
  precision_num <- which(precision_opts == precision)
  parts <- unlist(strsplit(version, "[.]"))
  parts[is.na(parts[1:precision_num])] <- "0"
  return(paste(parts[1:precision_num], collapse = "."))
}

I think any of these three versions (yours above, or these two) would be fine.

[kimyen]

This is great! Thank you so much @karawoo ! I will make the changes.

[karawoo]

Cool! I had another thought -- the message string in .printVersionOutOfDateWarnings() has our ran hardcoded in part of the string but not others. It might be good to generalize it to substitute in the package name and ran everywhere these appear

.printVersionOutOfDateWarnings <- function(current_version, latest_version, package, ran) {
  message <- sprintf(
    "\nNew %s version detected: You are using %s version %s. %s version %s is detected at %s. To upgrade to the latest version of synapser, please run the following command: install.packages(\"%s\", repos=\"%s\")\n",
    package,
    package,
    current_version,
    package,
    latest_version,
    ran,
    package,
    ran
  )
  packageStartupMessage(message)
}

If it's too hard to follow all the %s's this could use paste0() instead.

[kimyen]

Yeah, you're right. I missed that.