-
Notifications
You must be signed in to change notification settings - Fork 22
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
Fix tests, update contributors, prepare release #212
Conversation
replace with smaller routing.zip for some tests
time on cran reduced from 11.7s to 4.5s
@@ -1,3 +1,4 @@ | |||
data.table::setDTthreads(2) |
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.
Yes that is good as it covers all tests globally. You could play additional dances and condition on 'knowable' states (e.g. if (Sys.info()[["sysname"]]=="Linux)
may make sense) but this should work.
(Oh, and just to state the obvious in case there was any doubt: I disagree strongly with this Policy requirement but my few posts on the package development list have clearly not changed anybody's mind at CRAN so the above is chiefly a coping mechanism.)
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.
ok 👍 thanks!
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.
No worries, it was quite clear that you disagree with the policy decision.
I don't know enough about multithreading packages myself but I don't find the arguments made by CRAN in the R-pkg-devel list that I skimmed to be all that convincing. The way the policy is enforced however (I just learned of it because I wanted to push a really minor fix to CRAN) is, let's say, not ideal. So at this point, any coping mechanism is fine by me, I just wanted to get the package up again.
Prepare release 1.6.1. The package is on its way to CRAN now.
Resolving the following note was IMO unnecessary convoluted:
I found a solution here: Rdatatable/data.table#5658 as @eddelbuettel stated
definitely applies here.