Skip to content

Add PythonEngine Eval and Exec #389

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

Merged
merged 2 commits into from
Feb 24, 2017
Merged

Add PythonEngine Eval and Exec #389

merged 2 commits into from
Feb 24, 2017

Conversation

yagweb
Copy link
Contributor

@yagweb yagweb commented Feb 17, 2017

"eval" and "exec" are two of the most frequently used methods when interacting with Python. However, I found there is not "eval" provided currently, this statement var flag = (IntPtr)257; in PythonEngine.RunString method makes this method can only be used as "exec".

Two .NET functions equivalent to the Python “evel” and “exec” are implemented by wrapping the PyRun_String function just like the CPython does. PythonEngine.RunString method became internal.

All the modifications located in pythonengine.cs. All the places using PythonEngine.RunString are replaced with the new PythonEngine.Exec method.

Unit tests are given in the new file "src/embed_tests/pyrunstring.cs", They can also be used as examples. List one of them below.

Eval the python expression "sys.attr1 + a + 1" and obtain its value.

dynamic sys = Py.Import("sys");
sys.attr1 = 100;
PyDict locals = new PyDict();
locals.SetItem("sys", sys);
locals.SetItem("a", new PyInt(10));

var b = PythonEngine.Eval("sys.attr1 + a + 1", null, locals.Handle)
     .AsManagedObject(typeof(Int32));

Console.WriteLine("sys.attr1 + a + 1 = {0}", b);

CHANGELOG.md will be updated later.

Copy link
Contributor

@vmuriart vmuriart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks good. Left a couple comments in terms of design choice. I'd like to merge this after #391 to ensure no unexpected behavior from Travis.

/// RunString Method
/// </summary>
/// <remarks>
/// Run a string containing Python code. Returns the result of
/// executing the code string as a PyObject instance, or null if
/// an exception was raised.
/// </remarks>
public static PyObject RunString(
string code, IntPtr? globals = null, IntPtr? locals = null
internal static PyObject RunString(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this stay as a public method? Not sure how much its being used at the moment, but I wouldn't like to pull it off the API without first deprecating it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just re-read your statement on the pull_request, I understand this change now. I think I would still want to deprecate it first before removing it a release or two later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vmuriart I agree with deprecating this method first. When this method become internal, the type of its last argument _flag can be changed from enum RunFlagType to IntPtr, and the enum RunFlagType can be removed. In the Eval and Exec methods, it can calls this RunString method using Runtime.Py_eval_input and Runtime.Py_file_input for this argument.

public static PyObject Eval(string code, IntPtr? globals = null, IntPtr? locals = null)
{
PyObject result = RunString(code, globals, locals, RunFlagType.Eval);
return result;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the result be checked for exception before returning?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed, RunString already does that via Py.Throw.

public static void Exec(string code, IntPtr? globals = null, IntPtr? locals = null)
{
PyObject result = RunString(code, globals, locals, RunFlagType.File);
if (result.obj != Runtime.PyNone)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@filmor then on the flip-side, is this check here then superfluous?

Copy link
Contributor Author

@yagweb yagweb Feb 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vmuriart It can be removed. RunString method will always return None when passing the last parameter Runtime.Py_file_input. So this check failed only when there is a runtime bug of CPython.

@vmuriart
Copy link
Contributor

Just ran the tests on Travis after #391. PY27 fails with Fatal Python error: auto-releasing thread-state, but no thread-state for this thread which is unrelated to these changes. Looks good overall.

Only changes would be to keep RunString public but add a deprecation warnings instead..

One of the tests is writing to stderr in the middle of the tests not sure if we can do much on it though.

@vmuriart vmuriart changed the title add eval and exec Add PythonEngine Eval and Exec Feb 23, 2017
@vmuriart vmuriart force-pushed the add_eval branch 2 times, most recently from 2886267 to bcadc48 Compare February 23, 2017 22:43
-  Deprecation/Removal should be a separate issue/pr
-  In NUnit/XUnit, expected is the first argument, actual is second. Opposite to how Python does it
@vmuriart vmuriart merged commit 9bf4239 into pythonnet:master Feb 24, 2017
@@ -24,10 +24,10 @@ public void Dispose()
public void TestRunSimpleString()
{
int aa = PythonEngine.RunSimpleString("import sys");
Assert.AreEqual(aa, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yagweb I think you had these backwards by mistake, its the opposite other than python. I fixed them before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants