Skip to content

Comments

Rebased PR 450 (Py_Initialize without overriding signals)#724

Closed
filmor wants to merge 7 commits intopythonnet:masterfrom
filmor:449_Py_Initialize_SignalConfiguration
Closed

Rebased PR 450 (Py_Initialize without overriding signals)#724
filmor wants to merge 7 commits intopythonnet:masterfrom
filmor:449_Py_Initialize_SignalConfiguration

Conversation

@filmor
Copy link
Member

@filmor filmor commented Aug 21, 2018

@rmadsen-ks

What does this implement/fix? Explain your changes.

Avoiding overriding signals from the application so that CTRL-C might be handled.

Using Py_InitializeEx(0) (instead Py_Initialize) to avoid overriding signals.

Does this close any currently open issues?

#449 #450

Any other comments?

Just a rebased version of #450.

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

@filmor filmor requested review from den-run-ai and dmitriyse August 21, 2018 14:28
@den-run-ai
Copy link
Contributor

den-run-ai commented Aug 22, 2018

@filmor why is this failing CI?

@filmor
Copy link
Member Author

filmor commented Aug 22, 2018

Something about CSharpCodeProvider, I'll look into it. It's the same error that @rmadsen-ks had already when he pushed the PR, I think.

@filmor filmor mentioned this pull request Aug 30, 2018
Closed
4 tasks
@filmor filmor changed the title Rebased PR 450 Rebased PR 450 (Py_Initialize without overriding signals) Aug 30, 2018
@filmor filmor added this to the 2.4.0 milestone Aug 30, 2018
@codecov
Copy link

codecov bot commented Oct 16, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@e0f47ba). Click here to learn what that means.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #724   +/-   ##
=========================================
  Coverage          ?   75.58%           
=========================================
  Files             ?       60           
  Lines             ?     5627           
  Branches          ?      888           
=========================================
  Hits              ?     4253           
  Misses            ?     1060           
  Partials          ?      314
Flag Coverage Δ
#setup_linux 68.55% <ø> (?)
#setup_windows 74.56% <66.66%> (?)
Impacted Files Coverage Δ
src/runtime/runtime.cs 90.9% <0%> (ø)
src/runtime/pythonengine.cs 78.09% <100%> (ø)

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 e0f47ba...3d6024d. Read the comment docs.

@filmor filmor force-pushed the 449_Py_Initialize_SignalConfiguration branch from 168ec06 to e2b8c25 Compare October 29, 2018 12:47
@filmor filmor added the next label Oct 29, 2018
@rmadsen-ks
Copy link
Contributor

rmadsen-ks commented Nov 13, 2018

@filmor, the only change in this code is optionally calling InitializeEx instead of Initialize. This code is often being touched by tests. The actual change here is really simple, the unit test is excessively complex. Could you consider merging this without a unittest testing CTRL+C explicitly?

@filmor filmor force-pushed the 449_Py_Initialize_SignalConfiguration branch from cabeec8 to f9810f8 Compare November 13, 2018 12:21
@filmor
Copy link
Member Author

filmor commented Nov 14, 2018

I think you are right. I'll merge the PR without the unit test using a merge commit, that way the test is at least present in the history.

@filmor filmor mentioned this pull request Nov 15, 2018
Merged
@filmor filmor modified the milestones: 2.4.0, 3.0.0 Nov 15, 2018
@filmor filmor added test and removed next labels Nov 15, 2018
@filmor filmor force-pushed the 449_Py_Initialize_SignalConfiguration branch from e38124a to 3d6024d Compare November 15, 2018 13:17
@filmor filmor closed this Mar 6, 2019
@filmor filmor deleted the 449_Py_Initialize_SignalConfiguration branch March 6, 2019 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@den-run-ai den-run-ai Awaiting requested review from den-run-ai

@dmitriyse dmitriyse Awaiting requested review from dmitriyse

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@filmor @den-run-ai @rmadsen-ks