pkgsearch icon indicating copy to clipboard operation
pkgsearch copied to clipboard

add option to skip version check for cran_package()

Open ErdaradunGaztea opened this issue 2 years ago • 8 comments

Sometimes packages have version names that don't fit into is_package_version() requirements. For example, xtable, which consistently names its versions in 1.X-X pattern. These names are returned by cran_package_history() and I'd expect them to be usable in cran_package() no matter the form (in fact, I use this very pipeline in deepdep package).

Since I don't want to break backwards compatibility, I added check_version argument with default TRUE value that allows the user to skip is_package_version() check. Documented and tested the behaviour to make sure everything works.

Side note, 4 tests fail now (and they did before I changed anything in the code), because igraph is no more the top search result, it's crayon now.

ErdaradunGaztea avatar Dec 14 '21 16:12 ErdaradunGaztea

Thanks! This is however a bug, pkgsearch:::is_package_version should not fail for valid version numbers.

gaborcsardi avatar Dec 14 '21 18:12 gaborcsardi

Honestly, as mentioned in #39, valid version numbers can be almost anything. While there are guidelines for semantical versioning, not all packages follow them and, ultimately, the package's author has the last say on how to name the version. Thus, the implementation would basically look like is_package_version <- function(pkg) TRUE (maybe except for checking if pkg is a single string).

ErdaradunGaztea avatar Dec 14 '21 18:12 ErdaradunGaztea

Honestly, as mentioned in #39, valid version numbers can be almost anything.

For very old packages, yes. Otherwise in the past 20 years or so they have to look like this:

❯ base::.standard_regexps()$valid_package_version
[1] "([[:digit:]]+[.-]){1,}[[:digit:]]+"

But yeah, we can just drop that check, and only check if it is a non-NA string scalar.

gaborcsardi avatar Dec 14 '21 19:12 gaborcsardi

Ahh, didn't know that, interesting! I'm on board with dropping that, should I implement this check?

ErdaradunGaztea avatar Dec 14 '21 19:12 ErdaradunGaztea

YEs, please. Thanks! Just have an is_string instead, where is_string is like desc:::is_string.

gaborcsardi avatar Dec 14 '21 20:12 gaborcsardi

I saw you using assertthat, so I reused their function is.string() with added (also theirs) noNA() check. Also, dropped the check_version parameter, since the check itself has to pass now for the call to make sense.

ErdaradunGaztea avatar Dec 14 '21 20:12 ErdaradunGaztea

Checking the failing tests...

  • [x] cran_package_history() fails because v1.2.10 of igraph was published and the sorting would place it between v1.2.1 and v1.2.2; I know it's related to #39, I've written a quick package for that (versionsort) because existing functions aren't doing all that's needed
  • [x] I believe cran_package() fails on Windows because the sorting places lowercase fields at the back, though I have no way of confirming that, as my Windows machine doesn't do this

ErdaradunGaztea avatar Dec 15 '21 11:12 ErdaradunGaztea

Okay, I believe I might have fixed failing tests. I went as far as submitting versionsort to CRAN, but for now I copied version sorting code into R/versort_utils.R.

ErdaradunGaztea avatar Dec 15 '21 19:12 ErdaradunGaztea

Thanks, and I am sorry for the long wait. I ended up solving this a different way.

gaborcsardi avatar Oct 19 '23 08:10 gaborcsardi