- Notifications
You must be signed in to change notification settings - Fork751
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
👍 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.
src/runtime/pythonengine.cs Outdated
| /// 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( |
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.
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.
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 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.
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.
@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; |
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.
Should the result be checked for exception before returning?
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.
Not needed,RunString already does that viaPy.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) |
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.
@filmor then on the flip-side, is this check here then superfluous?
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.
@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.
Just ran the tests on Travis after#391. PY27 fails with Only changes would be to keep One of the tests is writing to stderr in themiddle of the tests not sure if we can do much on it though. |
2886267 tobcadc48Compare- 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
src/embed_tests/pyrunstring.cs Outdated
| @@ -24,10 +24,10 @@ public void Dispose() | |||
| public void TestRunSimpleString() | |||
| { | |||
| int aa = PythonEngine.RunSimpleString("import sys"); | |||
| Assert.AreEqual(aa, 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.
@yagweb I think you had these backwards by mistake, its the opposite other than python. I fixed them before merge.
Uh oh!
There was an error while loading.Please reload this page.
"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.
CHANGELOG.md will be updated later.