Add pysetargv by filmor · Pull Request #347 · pythonnet/pythonnet · GitHub

What does this implement/fix? Explain your changes.

Implements an overload of Initialize and a Py.SetArgv function that allow the user to issue a PySys_SetArgvEx call. By default, this call is done on Initialize with 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-shutdown branch as otherwise I'm not able to run the unit-tests reliably..

This replaces #301?

Can this be #if PYTHON3 ?

Yes, I'll change it.

Yes, it's an alternative using directly the Python API function.

first need to check that it is not null

Why? If it is null, this will fail with a NullReferenceException, perfectly fine. This PR is supposed to ensure that sys.argv is always initialised.

this gets overwritten by setup.py or specific needs of the user, so no need to update this

He probably added that during his development and forgot to remove it.

Updated #346 to include configuration for PY3 development.

Yep, exactly that, I'll remove this line.

I'm OK with merging this version as long as comments are addressed

Checks whether an error occurred and in case it has throws a PythonException.

In particular in Py.Import.

@filmor rebased your work here

https://github.com/pythonnet/pythonnet/tree/pysetargv

Codecov Report

Merging #347 into master will increase coverage by -0.08%.

@@            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

Impacted Files	Coverage Δ
src/runtime/runtime.cs	81.32% <ø> (ø)	✅
src/runtime/pythonengine.cs	54.16% <36.36%> (-1.18%)	❌
src/runtime/managedtype.cs	46.15% <ø> (ø)	✅
src/runtime/delegatemanager.cs	87.6% <ø> (ø)	✅
src/runtime/converter.cs	75.38% <ø> (ø)	✅
src/runtime/interfaceobject.cs	43.75% <ø> (ø)	✅
src/runtime/classderived.cs	0% <ø> (ø)	✅
src/runtime/pyobject.cs	22.22% <ø> (ø)	✅
src/runtime/exceptions.cs	72.8% <ø> (ø)	✅

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 f7f2fc0...3785c40. Read the comment docs.

Closes #347

@filmor the coverage on pythonengine.cs reduced

It's probably the refactoring he did. The coverage needs some fine tuning, its definetly not as robust as python's coverage engine.

where m_free is set?

why is this now IDisposable?

Because that's easier to use correctly than Initialize and Shutdown.

why not also check for IntPtr.Zero?

Because this function either returns null and sets an exception, or it returns a module.

don't we need to check if sys.argv is non-empty in the case of extending with import clr? @filmor @vmuriart anyway let's see if this passes my tests

i did not click submit review few weeks ago

vmuriart reviewed Jan 31, 2017

View reviewed changes

den-run-ai reviewed Jan 31, 2017

View reviewed changes

filmor force-pushed the add-pysetargv branch from ebbdb35 to 174e1fa Compare January 31, 2017 21:17

filmor added 5 commits January 31, 2017 15:08

Implement Py.SetArgv.

eca5ac9

Add a Py.Throw function.

331524f

Checks whether an error occurred and in case it has throws a PythonException.

Add overload for Initialize to pass specific args.

a0849a9

Allow the engine to be initialized as a Disposable.

4f67c5d

Use Py.Throw in a few more places.

3785c40

In particular in Py.Import.

vmuriart force-pushed the add-pysetargv branch from 174e1fa to 3785c40 Compare February 1, 2017 01:42

vmuriart merged commit 3785c40 into pythonnet:master Feb 1, 2017

vmuriart added a commit that referenced this pull request Feb 1, 2017

Merge pull request #347 from filmor/add-pysetargv

7cab332

Closes #347

filmor mentioned this pull request Feb 1, 2017

Ensure that sys.argv is set #301

Merged

den-run-ai mentioned this pull request Feb 24, 2017

Compatibility issue between argparse and pythonnet packages #404

Closed

den-run-ai reviewed Feb 24, 2017

View reviewed changes

filmor deleted the add-pysetargv branch March 6, 2019 16:20

                   <Optimize>false</Optimize>
                   <DebugType>full</DebugType>
                   <PlatformTarget>x64</PlatformTarget>
+                  <DefineConstants>TRACE;DEBUG;PYTHON3;PYTHON35;UCS2</DefineConstants>

                           int size = name + ascii.Length + 1;
                           IntPtr ptr = Marshal.AllocHGlobal(size);
-                          for (int i = 0; i <= m_free; i += IntPtr.Size)
+                          for (int i = 0; i < m_free; i += IntPtr.Size)

                   /// This class provides the public interface of the Python runtime.
                   /// </summary>
-                  public class PythonEngine
+                  public class PythonEngine : IDisposable

+                          using (new PythonEngine())
+                          using (var argv = new PyList(Runtime.Runtime.PySys_GetObject("argv")))
+                          {
+                              Assert.That(argv.Length() != 0);

-                          {
-                              throw new PythonException();
-                          }
+                          Py.Throw();

+                      {
+                          Initialize(Enumerable.Empty<string>());
+                      }

		PyImport_GetModuleDict();


		#if !(PYTHON26 \|\| PYTHON27)

Add pysetargv by filmor · Pull Request #347 · pythonnet/pythonnet · GitHub

Uh oh!

Add pysetargv - #347

Add pysetargv#347vmuriart merged 5 commits intopythonnet:masterpythonnet/pythonnet:masterfrom filmor:add-pysetargvfilmor/pythonnet:add-pysetargvCopy head branch name to clipboard

Conversation

filmor commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Uh oh!

vmuriart commented Jan 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filmor commented Jan 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filmor Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

den-run-ai commented Jan 31, 2017

Uh oh!

vmuriart commented Jan 31, 2017

Uh oh!

codecov-io commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

den-run-ai commented Feb 1, 2017

Uh oh!

vmuriart commented Feb 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add pysetargv#347
vmuriart merged 5 commits into
pythonnet:masterfrom
filmor:add-pysetargv

filmor commented Jan 31, 2017 •
edited
Loading

filmor Jan 31, 2017 •
edited
Loading

codecov-io commented Feb 1, 2017 •
edited
Loading