- Notifications
You must be signed in to change notification settings - Fork768
Add pysetargv#347
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Add pysetargv#347
Uh oh!
There was an error while loading.Please reload this page.
Conversation
vmuriart commentedJan 31, 2017
This replaces#301? |
src/runtime/runtime.cs Outdated
| PyImport_GetModuleDict(); | ||
| #if!(PYTHON26||PYTHON27) |
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.
Can this be#if PYTHON3 ?
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, I'll change it.
filmor commentedJan 31, 2017
Yes, it's an alternative using directly the Python API function. |
| using(newPythonEngine()) | ||
| using(varargv=newPyList(Runtime.Runtime.PySys_GetObject("argv"))) | ||
| { | ||
| Assert.That(argv.Length()!=0); |
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.
first need to check that it is not null
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.
Why? If it isnull, this will fail with aNullReferenceException, perfectly fine. This PR is supposed to ensure thatsys.argv is always initialised.
src/runtime/Python.Runtime.csproj Outdated
| <Optimize>false</Optimize> | ||
| <DebugType>full</DebugType> | ||
| <PlatformTarget>x64</PlatformTarget> | ||
| <DefineConstants>TRACE;DEBUG;PYTHON3;PYTHON35;UCS2</DefineConstants> |
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.
this gets overwritten by setup.py or specific needs of the user, so no need to update this
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.
He probably added that during his development and forgot to remove it.
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.
Updated#346 to include configuration for PY3 development.
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.
Yep, exactly that, I'll remove this line.
den-run-ai commentedJan 31, 2017
I'm OK with merging this version as long as comments are addressed |
Checks whether an error occurred and in case it has throws aPythonException.
In particular in Py.Import.
vmuriart commentedJan 31, 2017
@filmor rebased your work here |
codecov-io commentedFeb 1, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report@@ Coverage Diff @@## master #347 +/- ##==========================================- Coverage 61.45% 61.37% -0.08%========================================== Files 61 61 Lines 5336 5369 +33 Branches 900 896 -4 ==========================================+ Hits 3279 3295 +16- Misses 1819 1853 +34+ Partials 238 221 -17
Continue to review full report at Codecov.
|
den-run-ai commentedFeb 1, 2017
@filmor the coverage on pythonengine.cs reduced |
vmuriart commentedFeb 1, 2017
It's probably the refactoring he did. The coverage needs some fine tuning, its definetly not as robust as python's coverage engine. |
| intsize=name+ascii.Length+1; | ||
| IntPtrptr=Marshal.AllocHGlobal(size); | ||
| for(inti=0;i<=m_free;i+=IntPtr.Size) | ||
| for(inti=0;i<m_free;i+=IntPtr.Size) |
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.
where m_free is set?
| /// This class provides the public interface of the Python runtime. | ||
| /// </summary> | ||
| publicclassPythonEngine | ||
| publicclassPythonEngine:IDisposable |
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.
why is this now IDisposable?
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.
Because that's easier to use correctly thanInitialize andShutdown.
| { | ||
| thrownewPythonException(); | ||
| } | ||
| Py.Throw(); |
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.
why not also check for IntPtr.Zero?
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.
Because this function either returns null and sets an exception, or it returns a module.
| { | ||
| Initialize(Enumerable.Empty<string>()); | ||
| } | ||
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.
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.
i did not click submit review few weeks ago
Uh oh!
There was an error while loading.Please reload this page.
What does this implement/fix? Explain your changes.
Implements an overload of
Initializeand aPy.SetArgvfunction that allow the user to issue aPySys_SetArgvExcall. By default, this call is done onInitializewith a list that contains a single empty string (['']), using the arguments supplied to the .NET process if available.Does this close any currently open issues?
#299.
Any other comments?
Based on the
fix-shutdownbranch as otherwise I'm not able to run the unit-tests reliably..