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

Change rgl.* calls to *3d calls. #174

Merged
merged 3 commits into from
Jan 6, 2023
Merged

Conversation

dmurdoch
Copy link
Contributor

@dmurdoch dmurdoch commented Jan 6, 2023

The surface3d() function needs an rgl fix that's in the about-to-be-submitted version 0.111.5, so I've added that version requirement. Let me know if you'd prefer a run-time test and workaround for the old code.

…l fix in the about-to-be-submitted version 0.111.5, so I've added that version requirement.
@eddelbuettel
Copy link
Owner

Thanks for the changes. I am always very hesitant to add Suggests: rgl (>= 0.111.5) when such a version is not yet on CRAN and personally prefer to test in code if the required version is enough, and politely exit with a user-facing note when not.

Here it may not matter as the CI default is to ignore Suggests: (as I commonly do, that is a long story in and by itself you also correctly harped upon on one of the lists (== we collectively agree to minimize test surface here to keep tests in time limits, which is somewhat contrary to having tests in the first place yada yada ... but CRAN time is finite yada yada).

@eddelbuettel
Copy link
Owner

The one thing I always ask about is to add an extry to ChangeLog. Can you do that? Standard old school GNU format with date, name, email separated by two spaces, bullets indented by eight or a tab.

@dmurdoch
Copy link
Contributor Author

dmurdoch commented Jan 6, 2023

This commit should work with current CRAN rgl as well as the upcoming one. After 0.111.5 is released, the version check code could be removed.

DESCRIPTION Outdated Show resolved Hide resolved
@eddelbuettel
Copy link
Owner

eddelbuettel commented Jan 6, 2023

Sorry for the hoops but I cannot undo 'red' if you mark the issue as resolved, methinks. If I raised maybe they want me to clear it:

Never mind, my bad.

Nice PR otherwise, thanks for all the work.

@eddelbuettel eddelbuettel merged commit 817b0b4 into eddelbuettel:master Jan 6, 2023
@dmurdoch
Copy link
Contributor Author

dmurdoch commented Jan 6, 2023

Thanks!

@dmurdoch dmurdoch deleted the rgl branch January 6, 2023 17:51
@eddelbuettel
Copy link
Owner

I got distracted waiting for the CI and then forgot about the pending PR -- thanks again. The versioned check is perfect.

I am about to commit a simple fix (to create a default options array object) so that calling the viewer will work.

@dmurdoch
Copy link
Contributor Author

dmurdoch commented Jan 7, 2023

The newer version of rgl is now on CRAN, so if you prefer a version requirement in DESCRIPTION that would work now, or you could leave in the run-time check and let people keep working with older versions.

@eddelbuettel
Copy link
Owner

Thanks, I saw via CRANberries so congrats on a smooth transition. I think the run-time check should do.

@eddelbuettel
Copy link
Owner

I got a little tied up with various other package updates (and still have some to do...) but I reckoned rgl, long updated at CRAN, is fine without a RQuantLib update as you rejigged the example to work with old and new rgl, correct?

@dmurdoch
Copy link
Contributor Author

I think the RQuantlib changes should work fine with what's on CRAN now, and should be fine after the next update too. The most recent update on CRAN doesn't do the deprecation, the next one will. I've just finished going through the revdeps on CRAN to let authors know they'll get deprecation warnings as of the next release. I'm not sure when that will happen, but it'll likely be at least a week, more likely two.

@eddelbuettel
Copy link
Owner

Ah, that explains it -- I wasn't looking closely enough.

Especially once I have my "big fish" out of the way (Rcpp has been uploaded and I am waiting for 1 1/2 days now, RcppArmadillo and then BH are next) it would be a quick turnaround for this package. Just let me if/when you need an update.

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.

None yet

2 participants