Refactor python engines api #14940
Refactor python engines api #14940
Conversation
UI Smoke TestsTest: success. 2 passed, 0 failed. |
|
@aparajit-pratap @mjkkirschner |
no, @aparajit-pratap only added files for DynamoCore I think and we have yet to move any members to the shipped list. We need to schedule work to make use of this new system. We also need to decide about using warnings as errors. |
src/NodeServices/PythonServices.cs
Outdated
| /// Use this function to customize Python engine initialization. | ||
| /// This function will be called only once on all existing or future python engines (on PythonEngineAdded). | ||
| /// </summary> | ||
| public Action<PythonEngine> CustomizeEngine |
There was a problem hiding this comment.
this seems kind of dangerous, I imagine you want integrations like Revit to use this to setup marshallers and hook up events?
But this would also be easily accessible to anyone since this class has a static instance method - it could easily break python integration.
src/NodeServices/PythonServices.cs
Outdated
|
|
||
| /// <summary> | ||
| /// Use this function to customize Python engine initialization. | ||
| /// This function will be called only once on all existing or future python engines (on PythonEngineAdded). |
There was a problem hiding this comment.
comment is a bit misleading as this setter has the side effect of calling the method...
There was a problem hiding this comment.
Yeah, I kind of don't like a setter doing more than just replacing a value,
Still not convinced this is the proper way to go...
src/NodeServices/PythonServices.cs
Outdated
| /// <summary> | ||
| /// Event that is triggered for every engine that is removed. | ||
| /// </summary> | ||
| public static PythonEngineChangedHandler PythonEngineRemoved; |
There was a problem hiding this comment.
how do you imagine removal will work? Just remove the instance from the list of engines or actually try to unload it?
There was a problem hiding this comment.
With ALCs, I would imagine completely unloading it (assuming it works for python runtime)
Purpose
(FILL ME IN) This section describes why this PR is here. Usually it would include a reference to the tracking task that it is part or all of the solution for.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Mandatory section
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of