ropensci / rzmq

R package for ZMQ
https://docs.ropensci.org/rzmq
84 stars 30 forks source link

Install signal handler instead of IGN (#44) #45

Closed mschubert closed 6 years ago

mschubert commented 6 years ago

Pull request for https://github.com/ropensci/rzmq/issues/44.

work in progress

codecov-io commented 6 years ago

Codecov Report

Merging #45 into master will increase coverage by 0.37%. The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #45      +/-   ##
=========================================
+ Coverage   33.82%   34.2%   +0.37%     
=========================================
  Files           4       4              
  Lines         680     690      +10     
=========================================
+ Hits          230     236       +6     
- Misses        450     454       +4
Impacted Files Coverage Δ
src/interface.cpp 29.4% <71.42%> (+0.6%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 30d78ab...a66678a. Read the comment docs.

armstrtw commented 6 years ago

thanks.

mschubert commented 6 years ago

Thank you, but I'm afraid the merge as a bit too quick

Please do not put this on CRAN yet, it needs more testing (and likely a couple of adjustments)

jeroen commented 6 years ago

Please be careful not to override R's internal signal handing. R has a built-in methods for catching SIGINT which is exposed via the R_CheckUserInterrupt() api. I think you should only use custom signal handlers if you are resetting them at the end of the function.

mschubert commented 6 years ago

Yes, that part is still missing - hence the work in progress

jeroen commented 6 years ago

OK I'm going to revert it in master :)

jeroen commented 6 years ago

I have removed the commit from the master branch. Can you send a new PR once it is done? Maybe keep the WIP in your private repo :)

mschubert commented 6 years ago

Ha, I thought that was a good way to have you two have a look at the current state, but next time I'll put a

DO NOT MERGE YET

in bold and blinking letters ;)

WIP is here: https://github.com/mschubert/rzmq/tree/signal