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

Example robust to NULL osVersion #121

Merged
merged 2 commits into from
Nov 2, 2024

Conversation

MichaelChirico
Copy link
Contributor

@MichaelChirico MichaelChirico commented Nov 1, 2024

Recently ran into some packages having trouble with utils::osVersion being NULL (as is a documented possibility).

Now I am searching around GitHub a bit to see if I can't improve robustness in some other places too.

That led me to this change: if osVersion is NULL, startsWith() will return logical() and trip up if(). isTRUE() as done here is the simplest fix.

  • This is the only usage of osVersion in the package
  • I also note just above the condition exists("osVersion"). As osVersion was introduced in R 3.5.0, it wouldn't be so "weird" for it to be absent there 😉. I am not sure if you care about this dotted-dotted-line implicit R version dependency.

@eddelbuettel
Copy link
Owner

That's an interesting one! As you can imagine, installRub.r only works on Ubuntu so this check is relatively new and rare. Generalising it is easy so thanks for the PR.

[ Wasn't the "public word" that your in-house distro switched from Ubuntu-based to Debian-based so maybe you should stay away from Ubuntu binaries ... Anyway. ]

[ Also, I am happy to accommodate an endless stream of patches for your in-house special situation but at some point you guys may look into in-house firepower to take care of your needs. Just saying ... (as a share holder to boot ...) relying on unpaid external volunteers is a bit weird. ]

@eddelbuettel eddelbuettel merged commit f0edb0b into eddelbuettel:master Nov 2, 2024
1 check passed
@MichaelChirico MichaelChirico deleted the patch-1 branch November 2, 2024 01:19
@MichaelChirico
Copy link
Contributor Author

I didn't actually run into any issue from littler in this case. This one comes from searching CRAN & glancing at what I found:

https://github.com/search?q=author%3Amichaelchirico+is%3Apr+osVersion&type=issues

if you have a threshold below which you'd rather not be bothered with this type of janitorial PR, let me know & I can try and calibrate what to just let slide.

@eddelbuettel
Copy link
Owner

eddelbuettel commented Nov 2, 2024

No I love'em when they are clean and focussed. So thank you for those, and thank you for indulging in my habit of maintaining a ChangeLog file.

That said, we're both economists by training and it's hard to let go of comparing marginal cost and marginal benefit. My cost is here is low, but was there a benefit?

@MichaelChirico
Copy link
Contributor Author

There's an interesting discussion to be had there for sure. I know it passes my own cost/benefit test, especially being able to send the same fix to 9 packages in ~20 minutes (scaling up the very minimal benefit at very little marginal cost). I am cognizant of the overhead on review time though -- it's harder to know other maintainers' preferences at scale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants