-
Notifications
You must be signed in to change notification settings - Fork 381
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
feat: add support for comparing CRAN versions #656
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #656 +/- ##
==========================================
+ Coverage 77.99% 78.04% +0.05%
==========================================
Files 81 82 +1
Lines 5825 5852 +27
==========================================
+ Hits 4543 4567 +24
- Misses 1082 1084 +2
- Partials 200 201 +1 ☔ View full report in Codecov by Sentry. |
for (version in affected$versions) { | ||
tryCatch( | ||
{ | ||
as.package_version(version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does as
here mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand (which is very little in this space 😅) it's R's version of casting: https://www.rdocumentation.org/packages/methods/versions/3.0.3/topics/as
So R has this package_version
class, and we can make one of them by doing as.package_version(...)
which... apparently we can also do by just calling package_version(...)
?
It sounds like among other things as
is more performant as it does a quick return if the arg is already the right type.
That shouldn't be useful here so technically we could remove it but I think it reads a bit better since what we're doing here is really calling a constructor and discarding the result which is usually silly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice!!
also just a note: from what I can tell this is actually more "R" rather than "CRAN" aka this is exactly the same for the Bioconductor ecosystem, but I've not worried about that at all because 1. I think it should be pretty easy if just slightly "special-case-y" to handle if/when there's Bioconductor advisories (this will also apply to #668) |
Part of #642
See G-Rath/osv-detector#235 for the journey I went on with R for this