Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add option to skip version check for cran_package() #108

Closed
wants to merge 6 commits into from

Conversation

ErdaradunGaztea
Copy link

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.

@gaborcsardi
Copy link
Contributor

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

@gaborcsardi gaborcsardi added the bug an unexpected problem or unintended behavior label Dec 14, 2021
@ErdaradunGaztea
Copy link
Author

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).

@gaborcsardi
Copy link
Contributor

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.

@ErdaradunGaztea
Copy link
Author

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

@gaborcsardi
Copy link
Contributor

gaborcsardi commented Dec 14, 2021

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

@ErdaradunGaztea
Copy link
Author

ErdaradunGaztea commented Dec 14, 2021

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
Copy link
Author

ErdaradunGaztea commented Dec 15, 2021

Checking the failing tests...

  • 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 Some packages have NULL versions #39, I've written a quick package for that (versionsort) because existing functions aren't doing all that's needed
  • 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
Copy link
Author

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.

@gaborcsardi
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants