Add references to new DSIronPython package version #14752
Add references to new DSIronPython package version #14752mjkkirschner merged 4 commits intoDynamoDS:masterfrom
Conversation
| { | ||
| //allows us to access some default pref values of current version before prefs are | ||
| //actually written to disk(firstRun). | ||
| var currentVersionTempPrefs = new PreferenceSettings(); |
There was a problem hiding this comment.
I am not thrilled with this solution, but I am out of bandwidth to do any other refactoring, and this lets me add a test, the alternative is no test, and I can revert the version in DynamoModel.Start after the migrator has exited. (see the previous commit).
@saintentropy @pinzart90 do you see an issue with creating temp prefs like this?
There was a problem hiding this comment.
I see... There is no good way to access the default. If you pull it out then we might forgot to sync them later correct?
There was a problem hiding this comment.
yeah, it's probably not a big deal to add 3.0.0 inline, but theres already 2 or 3 different ways we avoid migration in various places, I'd like to avoid yet another way with a random version number that we'll forget to update.
Longterm I think this class needs a refactor / improvement.
UI Smoke TestsTest: success. 2 passed, 0 failed. |
UI Smoke TestsTest: success. 2 passed, 0 failed. |
| /// if found at deserialize time. | ||
| /// </summary> | ||
| internal static Version ironPythonResolveTargetVersion = new Version(2, 4, 0); | ||
| internal Version ironPythonResolveTargetVersion = new Version(3, 0, 0); |
There was a problem hiding this comment.
And you don't want to leave this static because ...
There was a problem hiding this comment.
if it's static when the migrator loads the old preferences it modifies the new preferences and we never even have a chance of looking up the value in the current version prefs.
I don't think it makes sense for any preferences to be backed by static fields.
There was a problem hiding this comment.
It seems like the reason it was made static was about access (ie not having to pass it in). From Martins PR
In order to make system and user settings available on the node, and
avoiding further changes to the API, the properties were made static
and internal.
There was a problem hiding this comment.
thanks, it was already easily available in the extension where it was needed AFAIK.
There was a problem hiding this comment.
But yes not having to read properties from the file and then deserialize this makes sence.
UI Smoke TestsTest: success. 2 passed, 0 failed. |
UI Smoke TestsTest: success. 2 passed, 0 failed. |
| var ironPythonVersion = new Version(3, 0, 0); | ||
| if(LoadedParams.StartupParams.Preferences is PreferenceSettings prefs) | ||
| { | ||
| Version.TryParse(prefs.IronPythonResolveTargetVersion,out ironPythonVersion); |
There was a problem hiding this comment.
Don't we have Internal visible set for PythonMigrationViewExtension. Can you access the internal field?
There was a problem hiding this comment.
Not a big deal... This works too.
UI Smoke TestsTest: success. 2 passed, 0 failed. |
UI Smoke TestsTest: success. 2 passed, 0 failed. |
3 participants
Purpose
When a user opens an old graph with IronPython nodes in it and no IronPython engine, we currently add a reference to DSIronPython2.7 version 2.4 - that package will no longer work in Dynamo 3.x
This PR does the following:
I am not really sure why this is in preferences at all...
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